jakartaee / servlet

Jakarta Servlet
https://eclipse.org/ee4j/servlet
Other
253 stars 81 forks source link

Add capability of ordering ServletContainerInitializer invocations #548

Closed mnriem closed 1 month ago

mnriem commented 9 months ago

The Servlet 3.0 and 3.1 specs require no specific order for ServletContainerInitializers. This is a serious flaw in the spec, because it eliminates the ability to guarantee that certain code runs before any other code (for example, initialization of logging frameworks). In Servlet 2.5 and earlier, you could guarantee a exact ordering of startup code, but you can't do that anymore. To be clear, the spec does not prohibit the container from ordering them in a certain way, it just doesn't specify an order that should be followed.

(Description by Nicholas Williams: https://bugs.eclipse.org/bugs/show_bug.cgi?id=416300)

arjantijms commented 9 months ago

@markt-asf @gregw @stuartwdouglas WDYT?

gregw commented 9 months ago

In Jetty we already have a mechanism to order and exclude SCIs (including those on the container classpath). Currently we do it with context attributes that can have an explicit order with wildcards. So a standard approach for this would be good.

mnriem commented 6 months ago

@gregw Do you have a proposal for this scope of work?

gregw commented 6 months ago

@gregw Do you have a proposal for this scope of work?

Not particularly. As I said, jetty uses context attributes, but that is driven more by not creating proprietary API than specific design.

It would be good to use the fragment ordering mechanism, but that's not sufficient for SCIs from the container path, not exclusions

mnriem commented 6 months ago

So can we formalize a proposal here?

mnriem commented 6 months ago

@gregw @arjantijms Can we formalize a proposal here?

arjantijms commented 5 months ago

See also: https://github.com/jakartaee/servlet/issues/79

arjantijms commented 5 months ago

Jetty config: https://eclipse.dev/jetty/documentation/jetty-12/operations-guide/index.html#og-ordering

arjantijms commented 5 months ago

What @janbartel did:

You can either specify an exact ordering of the ServletContainerInitializers by specifying their classnames in a comma separated list as the context attribute "org.eclipse.jetty.containerInitializerOrder".

You may have one instance of "" in the list which will match all of the ServletContainerInitializers not already named. Eg "com.foo.A, com.foo.B, , com.acme.X"

Alternatively, if no exact ordering is given, or if the ordering is simply "*", we will fallback to ordering them:

  1. all ServletContainerInitializers from the container's classpath
  2. all ServletContainerInitializers from WEB-INF/classes
  3. all ServletContainerInitializers from WEB-INF/lib according to any ordering established in web.xml, otherwise in random order.
mnriem commented 5 months ago

@arjantijms That solves the problem for Jetty. What about for others? How can we formalize this?

arjantijms commented 5 months ago

@mnriem The jetty approach is very usable when you don't control the initialisers. However, maybe we should first start with something that gives the initialisers themselves a level of control and statement of intention.

What about something like the following?

@Init(priority = 67.89)
public class FacesInitializer implements ServletContainerInitializer {
}

Or

@InitPriority(67.89)
public class FacesInitializer implements ServletContainerInitializer {
}

Thoughts? @janbartel Any input?

gregw commented 5 months ago

I'm not a big fan of absolute priorities like that. You can end up with 99.9999 vs 99.99999 as everybody tries to be first or last, especially when baked into the code rather than config. Before/After semantics is often better.

arjantijms commented 5 months ago

You can end up with 99.9999 vs 99.99999 as everybody tries to be first or last, especially when baked into the code rather than config.

It's more like a start really. Eventually you'd want to define ranges, like e.g.is done for Interceptors: https://javaee.github.io/javaee-spec/javadocs/javax/interceptor/Interceptor.Priority.html

Before/After semantics is often better.

Or depends as @BalusC suggested?

@LogicalName("Faces")
@LogicalDependsOn("CDI", "WebSocket")
public class FacesInitializer implements ServletContainerInitializer {
}

Or without the "logical" in the annotation names, or any other annotation. The above just shows the idea.

janbartel commented 5 months ago

We could re-use the ordering concepts from the web fragments?

mnriem commented 5 months ago

@janbartel Can you take the lead on this?

janbartel commented 5 months ago

@mnriem I'm travelling for the next 10 days, but after that I can throw a proposal together.

janbartel commented 4 months ago

@arjantijms I'm skeptical that annotations will work: at coding time, you cannot know the other SCIs that will be present in a webapp. Typically users combine a lot of libraries that contain SCIs and then want to either exclude some of them, or control the ordering amongst them.

BalusC commented 4 months ago

This is about SCIs coming from JEE spec. E.g. Faces, CDI, WebSocket. So there's a predefined set. User/3rd-party-library defined ones can safely be ignored because JEE impls usually won't depend on them.

gregw commented 4 months ago

This is about SCIs coming from JEE spec. E.g. Faces, CDI, WebSocket. So there's a predefined set. User/3rd-party-library defined ones can safely be ignored because JEE impls usually won't depend on them.

Is it really only about only SCIs from JEE specs?

If that was the case, then we could just specify an order of all the known SCIs and each container could implement the ordering privately with their own mechanism.

I think coming up with a generalize extensible mechanism for ordering is what this issue is about. I also think it is the most flexible and future proof way to go.

BalusC commented 4 months ago

Sorry I lost track of context. I actually meant that names like below, Faces, CDI and WebSocket can be predefined in JEE specs. The JEE impls won't care about user-defined SCIs.

@LogicalName("Faces")
@LogicalDependsOn("CDI", "WebSocket")

I personally think the package identifiers are better to represent names. E.g. jakarta.faces, jakarta.inject, jakarta.websocket.

In any case, naming them isn't a concern for the Servlet spec. It's just that the SCIs should be orderable somehow. Either via (new) annotations or via existing <ordering> in web-fragment.xml.

janbartel commented 4 months ago

@BalusC I'm confused. I thought we were going to define a general purpose ordering mechanism for SCIs, irrespective of whether they are JEE Spec or some other SCI? This would give users control over the ordering/exclusion of all SCIs present in the webapp and container.

arjantijms commented 4 months ago

This would give users control over the ordering/exclusion of all SCIs present in the webapp and container.

That would be nice, but we also really need something for ServletContainerInitializer implementations themselves to set this.

E.g. the Mojarra ServletContainerInitializer knows it depends on CDI and WebSocket. WebSocket may depend on X or Y, etc.

We don't want to put this burden on the user and ask the user to specify in their web.xml the order of those for each of their applications. That would be strange.

Additionally, the opaque string in Bauke's example does not tie Servlet to Jakarta EE. The idea of Servlet being part of Jakarta EE should be prevented of course, and an opaque string should guarantee that. Jakarta EE can give meaning to that string, and again, Servlet does not have to be a part of Jakarta EE or be tied to Jakarta EE there.

janbartel commented 4 months ago

@arjantijms and @BalusC you seem to be saying that this annotation will only be used by JEE spec ServletContainerInitializers, but once you've defined the annotation, it is a general purpose mechanism because it will be available for library and webapp developers to use.

How do you see this interacting with the existing web-fragment ordering mechanism?

Given this annotation on a ServletContainerInitializer:

@LogicalName("Something")
@LogicalDependsOn("Other1", "Other2")
public class SomethingInitializer implements ServletContainerInitializer {
}

If Other2 is in a web-fragment that is excluded by an ordering, what would you expect to happen? Is this a deployment error or is the SomethingInitializer skipped?

Does the ordering implied by these annotations override that of the web-fragment ordering for the purposes of invoking ServletContainerInitializers, or does the web-fragment ordering take precendence?

BalusC commented 4 months ago

you seem to be saying that this annotation will only be used by JEE spec

No. Only that the "web fragment" name such as "Faces", "CDI", "WebSocket" in above examples will be defined by JEE spec. The annotation itself should be provided by servlet spec and this is of course publicly available to the rest of the world.

that is excluded by an ordering

I'm not aware that "web fragments" provided by the container (Faces, CDI, WebSocket, etc) can be excluded via web.xml ordering mechanism. Is this currently already implemented?

To be clear, it was never intented that the proposed annotation and the web.xml ordering mechanism are required to be combined. It was either-or. So we have 2 options:

  1. introduce new annotation(s), as proposed, completely independent from the web.xml ordering mechanism.
  2. or, reuse existing web.xml ordering mechanism to rank SCIs as well (it currently already ranks SCLs).

We imagined that introducing annotations would be quite of a work, so we just reimagined that we could perhaps also reuse the existing web.xml ordering mechanism. But if that turns out to be capable of disabling/excluding container-provided modules, then we should probably fall back to the original proposal of new annotations. Do note that the "logical name" in the annotation does not need to represent the web fragment name per se. The servlet container should just consider it as an unique identifier of the SCI and merely use it to rank the SCIs.

mnriem commented 4 months ago

From my perspective this is about delivering a mechanism that should in the end deliver an ordered list of ServletContainerInitializers. Which then should be able to be used by the Jakarta Platform specification to mandate a specific ordering for components used therein. I do not expect the Servlet specification to mandate the EE ordering.

janbartel commented 4 months ago

you seem to be saying that this annotation will only be used by JEE spec

No. Only that the "web fragment" name such as "Faces", "CDI", "WebSocket" in above examples will be defined by JEE spec. The annotation itself should be provided by servlet spec and this is of course publicly available to the rest of the world.

OK. But to be clear, these aren't "web-fragment" names, as these are already defined in the web-fragment.xml, plus of course a single web fragment can contain multiple ServletContainerInitializers. So these are just logical names for ServleContainerInitializers.

that is excluded by an ordering

I'm not aware that "web fragments" provided by the container (Faces, CDI, WebSocket, etc) can be excluded via web.xml ordering mechanism. Is this currently already implemented?

I thought we just agreed that the proposed annotation would be publicly available and thus used by both webapp and framework developers? So we would have to solve both for the case where the ServletContainerInitializer is provided by the container, and for the case where it is provided in the webapp. Or are you saying that you want this annotation to have no effect if it is on a ServletContainerInitializer that is not provided by the container??

The spec mentions shared frameworks that are provided by the container, but also says that these frameworks can instead be present inside the webapp, where they are subject to be excluded by ordering. See Section 8.2.4 Shared Libraries/Runtimes Pluggability:

The framework jar file can also be bundled in WEB-INF/lib directory of the war file. If the ServletContainerInitializer is bundled in a JAR file inside the WEB-INF/lib directory of an application, its onStartup method will be invoked only once during the startup of the bundling application. If, on the other hand, the ServletContainerInitializer is bundled in a JAR file outside of the WEB-INF/lib directory, but still discoverable by the runtime’s service provider lookup mechanism, its onStartup method will be invoked every time an application is started. Implementations of the ServletContainerInitializer interface will be discovered by the runtime’s service lookup mechanism or a container specific mechanism that is semantically equivalent to it. In either case, ServletContainerInitializer services from web fragment JAR files that are excluded from an absolute ordering MUST be ignored, and the order in which these services are discovered MUST follow the application’s class loading delegation model.

Any ServletContainerInitializer that is inside a web-fragment (ie inside the webapp) can be excluded by an ordering (Section 8.2.2, para 1.d). So if one of these JEE spec "container provided" ServletContainerInitializers was placed into the webapp, it is subject to being excluded according to the Spec.

To be clear, it was never intented that the proposed annotation and the web.xml ordering mechanism are required to be combined. It was either-or. So we have 2 options:

  1. introduce new annotation(s), as proposed, completely independent from the web.xml ordering mechanism.
  2. or, reuse existing web.xml ordering mechanism to rank SCIs as well (it currently already ranks SCLs).

We imagined that introducing annotations would be quite of a work, so we just reimagined that we could perhaps also reuse the existing web.xml ordering mechanism. But if that turns out to be capable of disabling/excluding modules, then we should probably fall back to the original proposal of new annotations.

As the web.xml/web-fragment.xml ordering mechanism already exists I don't see how you could introduce a totally independent mechanism.

Do note that the "logical name" in the annotation does not need to represent the web fragment name per se. The servlet container should just consider it as an unique identifier of the SCI and merely use it to rank the SCIs.

Yes.

BalusC commented 4 months ago

but also says that these frameworks can instead be present inside the webapp, where they are subject to be excluded by ordering

Yeah that's correct. But I was talking about container-provided ones not about webapp-provided ones.

In that case, it's completely fine to reuse web fragment ordering for SCIs as well (as it currenly already does for SCLs).

gregw commented 4 months ago

Fragment ordering will never be enough to order SCIs as fragments can have multiple SCIs and SCIs provided by the container are never in fragments.

So we need a different ordering mechanism... so that is either invent something new, or use some other existing ordering. How about using JPMS dependencies? Isn't that what they are for afterall?

We can ask each SCI for its Module, which will include a list of the Modules it depends on. We can thus order SCIs by their JPMS ordering.

BalusC commented 4 months ago

fragments can have multiple SCIs

Then we can specify the order among them will be undefined. Let the fragment developer sort out by itself.

and SCIs provided by the container are never in fragments.

Indeed not yet. But it's just a matter of delivering a META-INF/web-fragment.xml file along with the JAR.

If this is going to make it too complicated in your opinion then we can also go for the annotation proposal. No problem. Whatever works the best for Servlet spec.

We can ask each SCI for its Module, which will include a list of the Modules it depends on. We can thus order SCIs by their JPMS ordering.

That would be perfect.

gregw commented 4 months ago

If this is going to make it too complicated in your opinion then we can also go for the annotation proposal. No problem. Whatever works the best for Servlet spec.

Inventing a new dependency mechanism in annotations is doable, but will never be as simple as one would think:

all of a sudden you are re-inventing OSGI.

So why not just go with JPMS? Is is a module dependency mechanism built into java. We can discover the module of every SCI and its dependencies (static or otherwise). Thus we can build a dependency graph and do a topological sort of all SCIs.

This will not handle intra module ordering (one module with multiple SCIs), but that should come from a single developer, so if there are multiple SCIs then they can have no dependency between them (else the developer could have created a single SCI and encoded that dependency).

Using JPMS would just need the spec to say it should be used and containers could implement it with no additional APIs or annotations.

BalusC commented 4 months ago

Go ahead with JPMS. This was frankly also the first thing we had in mind before creating this issue (at least Arjan and me). If it's no problem for Servlet spec to have ties to JPMS then go ahead.

gregw commented 4 months ago

@markt-asf @stuartwdouglas what do you think about using JPMS as the sort algorithm for SCIs?

This could be done stand alone as the only ordering, or in combination with fragment based ordering.

This would still not give a fully deterministic ordering, as there can be multiple SCIs within a module or fragment. If we want fully deterministic, we could fall back to alphabetical on the class name for any undefined orderings.

arjantijms commented 4 months ago

We can ask each SCI for its Module, which will include a list of the Modules it depends on. We can thus order SCIs by their JPMS ordering.

I did think about this, although I had some doubts about that.

The potential problem I was thinking about is twofold;

  1. It only works if there is a compile time dependency on some other module. This may not be the case, e.g. the ServletContainerInitializer may need to fetch an object from say JNDI or somewhere else put in there by another ServletContainerInitializer, with the type of this object not exported by the module containing the latter ServletContainerInitializer.
  2. The module expresses the dependencies of the entire module. The ServletContainerInitializer likely depends on only a subset of this. I'm not sure if that would be a problem in practice.
arjantijms commented 4 months ago
  • Who administers the logical names?
  • are they case sensitive?
  • how do you deal with homoglyphs?
  • can you depend only on logical names like "JSP" or can you depend on implementations "apacheJSP"?

I think all of these should not matter to the Servlet spec. It's an opaque string, and the comparison is on the Java String type. Whatever its content is is decided by the developers using it. It's essentially like roles. They are just strings too, and compared on equality. If they are equal, they are the same role.

  • what about depending on specific versions?

Well, JPMS doesn't specify specific versions either, but OSGi indeed does. When using OSGi (and/or something like Maven in the build phase) the version of a dependency as a whole is already set.

all of a sudden you are re-inventing OSGI.

We should indeed be careful about not reinventing things, although the use case here is somewhat different. OSGi is essentially about modules (jars, typically) exporting and importing packages and their versions wrt compile time dependencies.

Here it's more about the runtime needs of a single class.

Given this again:

@LogicalName("Something")
@LogicalDependsOn("Other1", "Other2")
public class SomethingInitializer implements ServletContainerInitializer {
}

Then defaults using the module IDs seem logical:

@LogicalName("jakara.faces")
@LogicalDependsOn("jakarta.cdi", "jakarta.websocket")
public class SomethingInitializer implements ServletContainerInitializer {
}

I just wonder if we would need to have those annotations anyway to allow customisation. Just thinking out loud.

gregw commented 4 months ago
  1. It only works if there is a compile time dependency on some other module. This may not be the case, e.g. the ServletContainerInitializer may need to fetch an object from say JNDI or somewhere else put in there by another ServletContainerInitializer, with the type of this object not exported by the module containing the latter ServletContainerInitializer.

JPMS allows for static (read as "optional") dependencies.

  1. The module expresses the dependencies of the entire module. The ServletContainerInitializer likely depends on only a subset of this. I'm not sure if that would be a problem in practice.

This is a feature not a bug. If my SCI depends on the services of module XYZ, it should not need to know how XYZ is initialized. The use of an SCI is essentially an implementation detail and may change over time. Far better to just say you are dependent on "jakarta.xyz" in the JPMS module info, then the compiler does all the work checking for cycles and existence etc.

gregw commented 4 months ago

Then defaults using the module IDs seem logical:

@LogicalName("jakara.faces")
@LogicalDependsOn("jakarta.cdi", "jakarta.websocket")
public class SomethingInitializer implements ServletContainerInitializer {
}

Firstly "Logical" is a really poor prefix. Do SCIs have Illogical names as well?

How about just a single annotation that gives direct dependencies on any SCIs found in the named modules. This would simplify the work for the container as a full JPMS dependency graph would not need to be expanded to handle transitives:

@DependsOn("jakarta.cdi", "jakarta.websocket")
public class SomethingInitializer implements ServletContainerInitializer {
}

Although I actually don't like "DependsOn" as that is duplicating a semantic that is already in the code (module-info). So how about:

@Before("jakarta.websocket")
@After("jakarta.cdi")
public class SomethingInitializer implements ServletContainerInitializer {
}

PROS of this:

CONS:

gregw commented 4 months ago

Actually, I'm not sure JPMS will work. As @janbartel has just pointed out to me, the SCIs are likely to be in an implementation module and not the API module. I don't think JPMS has the concept of service like OSGI does.

So perhaps we do need to invent a new name space afterall.... but I'd prefer annotations like:

@Name("acme.something")
@Before("jakarta.websocket")
@After("jakarta.cdi")
public class SomethingInitializer implements ServletContainerInitializer {
}
janbartel commented 4 months ago

I think we're firming up on a new annotation along the lines previous discussed. That being the case, I've put a bit more flesh around it for further discussion:

Purpose

We wish to create an ordering over SCIs from all of the following sources:

Proposal

The proposal is to base the ordering on the same algorithm used for web-fragment relative ordering. Note that any absolute or relative web-fragment ordering established for a webapp plays no role in the ordering of SCIs, other than to exclude SCIs from consideration as per the existing Servlet Specification.

Example Annotation

@Name ("blah")
@Before ("bah")
@After ("humbug")
public class Blah implements ServletContainerInitializer

Where: @Name value is a single identifier composed of a-z A-Z 0-9 @Before value is a comma separated string of identifiers as above @After value is a comma separated string of identifiers as above

Reserved @Names

"Others" Can only appear in one of a @Before or @After on a single SCI. @Before("Others) means to place the SCI first in ordering. @After("Others") means to place the SCI last in ordering If multiple SCIs have a @Before("Others") or @After("Others") the exact order is not deterministic. The JEE and Servlet Specifications may define other reserved names eg "CDI", "JSP", "Websocket".

Algorithm

  1. Find all SCIs visible in the classloader as per the Servlet Specification.
  2. For each SCI:
   if from excluded jar
       ignore
   if duplicate name
       deployment error
   otherwise
       map name -> ordering (if no @Name, name is the package qualified classname)
  1. Perform ordering When processing @Before and @After names, if no SCI in the map matches that name results in a deployment error. Circular references result in a deployment error. If no @Before/After then the order of SCIs not deterministic.

  2. Call SCIs in established order

arjantijms commented 4 months ago

Firstly "Logical" is a really poor prefix. Do SCIs have Illogical names as well?

I like that question and reasoning. Indeed, we should not have users thinking there are Illogical names. The "logical" prefix was to contrast it to the "real" name of the ServletContainerInitializer which is the classname.

But the annotation names were really just illustrative and I'm sure we can find better names.

mnriem commented 4 months ago

Seems to be overkill to me. What is wrong with a priority annotation? Easy to implement. And then also easy for the EE platform to use for its purposes. Any ServletContainerInitializer with a priority annotation is ordered according to it and any ServletContainerInitializer without comes after that in no defined order. If it is in web.xml or web-fragment.xml it does not have a priority either. That keeps it all inline to current behavior and it is easy to implement. Let's not over engineer this!

gregw commented 4 months ago

I'm not a big fan of absolute priorities like that. You can end up with 99.9999 vs 99.99999 as everybody tries to be first or last, especially when baked into the code rather than config. Before/After semantics is often better.

Seems to be overkill to me.

Remember that containers already have before/after/other based ordering for fragments. So this will not be significantly new code or behaviour.

What is wrong with a priority annotation?

As said previously: "You can end up with 99.9999 vs 99.99999 as everybody tries to be first or last, especially when baked into the code rather than config. Before/After semantics is often better."

Easy to implement.

Yeah, but as said it is a new implementation, not working from existing code.

And then also easy for the EE platform to use for its purposes.

Is it? Who is the central authority that says who is 0.50 and who is 0.60 etc. Without central coordination like that, then priorities will not work. With before/after, a new SCI can just say that it must be run after CDI and so long as CDI publishes a good name, then the dependency works without coordination.

mnriem commented 3 months ago

@gregw @BalusC @janbartel @arjantijms Any progress? Or are we not going to reach consensus here and I should close it out?

janbartel commented 3 months ago

@mnriem I think the next step for those that don't like the proposal that is on the table to make a fully fleshed counter proposal that satisfies the Purpose as stated in my proposal. Or are we not even agreed on the purpose of this?

mnriem commented 3 months ago

As long as there is annotation support and order is guaranteed I am fine with just about any proposal that makes it standard behavior across vendors.

mnriem commented 1 month ago

I am closing this one out as it did not make it in EE11. If any wants to try again for EE12 feel free to open a new issue and link to this issue if so desired.