Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify the handling of ServletContainerInitializers #5959

Merged
merged 17 commits into from
Feb 19, 2021

Conversation

janbartel
Copy link
Contributor

This work has come out of proposals on how to start/stop the embedded websocket setup.

Historically, there was only 1 way to create and start ServletContainerInitializers and that was via code in the jetty-annotations module, cooperating with code in the jetty-plus module. In a nutshell, jetty-annotations discovered all the SCIs, excluded and ordered them as necessary, and then created a holder-like class for each of them. The annotation scanning would discover classes that should be passed into the SCI.onStartup method, and remember them in the holder-like class (o.e.j.p.a.ContainerInitializer). As the final step of the AnnotationConfiguration.configure method, it would add a bean onto the WebAppContext which would call onStartup for each of the SCIs (via the holder-like class).

Then at some point, a further mechanism was added in the jetty-servlet package that allowed beans to be added that implemented the ServletContainerInitializerCaller interface - each would start an SCI.

There was no real connection between these 2 mechanisms, they pretty much were separate and parallel.

With the suggestion to add an api to ServletContextHandler to allow SCIs to be added directly, it seems past due time to see what we can do to unify these 2 different approaches, hence this PR.

In essence, this PR creates a proper Holder for ServletContainerInitializers in the jetty-servlet package, and then re-uses these in the jetty-annotation package.

There a 3 new methods exposed on ServletContextHandler, the 3rd of which is used by jetty-annotations code, and the others can be used by embedded code or xml etc:

public void addServletContainerInitializer(ServletContainerInitializer containerInitializer)
public void addServletContainerInitializer(ServletContainerInitializer sci, Class<?>... classes)
public void addServletContainerInitializer(ServletContainerInitializerHolder... sciHolders)

The first 2 methods wrap the SCI into a ServletContainerInitializerHolder. All 3 methods use the same underlying mechansim of adding a single "starter" bean that will start all of the ServletContainerInitializerHolders at the appropriate point in the startup sequence for the context.

Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel marked this pull request as ready for review February 15, 2021 11:05
jetty-annotations/src/main/java/module-info.java Outdated Show resolved Hide resolved
jetty-quickstart/src/main/java/module-info.java Outdated Show resolved Hide resolved
jetty-servlet/src/main/java/module-info.java Outdated Show resolved Hide resolved
* @param classes the Set of application classes.
* @see Initializer
*/
public void addServletContainerInitializer(ServletContainerInitializer sci, Class<?>... classes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this method, if we return the holder, onto which we can add the classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we keep this method: it is in the style of the other add* methods to provide option to pass in other args used by the equivalent *Holder class - look at the various addFilter methods which have args that are passed onto the FilterHolder via setters. It think having this method signature allows users to ignore the ServletContainerInitializerHolder if they want to rather than forcing them to deal with it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but I still would like to see the holder be returned from the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbordet yes, it is now.

Signed-off-by: Jan Bartel <janb@webtide.com>
jetty-servlet/src/main/java/module-info.java Outdated Show resolved Hide resolved
public void addStartupClasses(String... names)
{
for (String n : names)
_startupClassNames.add(n);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Collections.addAll().

Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to consider if any of this should be back ported to 9.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants