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

o.e.j.servlet.listener.ContainerInitializer does not respect servlet spec contract for ServletContainerInitializers #5834

Closed
janbartel opened this issue Dec 22, 2020 · 6 comments
Assignees

Comments

@janbartel
Copy link
Contributor

jetty 9.4.x

See #3706.

The premise of the org.eclipse.jetty.servlet.listener.ContainerInitializer according to the comments is to ease using ServletContainerInitializers in embedded scenarios. Unfortunately, it calls the SCI from a ServletContextListener: according to the spec, a ServletContextListener cannot add another ServletContextListener. Thus, an SCI called via the ContainerInitializer will not be able to add a ServletContextListener, as it is perfectly entitled to do.

@gregw
Copy link
Contributor

gregw commented Dec 22, 2020

One solution may be to use a jetty lifecycle listener instead of a ServletContextListener

@joakime
Copy link
Contributor

joakime commented Dec 22, 2020

for embedded-jetty, I would love to see an API like ...

ServletContextHandler context = new ServletContextHandler();
context.addServletContextInitializer(new MySCI());
// other init
context.start();

Then let Jetty work out the internals of how that should come to be.

@gregw gregw assigned lachlan-roberts and unassigned joakime Jan 12, 2021
@gregw
Copy link
Contributor

gregw commented Jan 12, 2021

@lachlan-roberts I think you are already addressing this one in #5840
I think you are just deprecating it rather than fixing it... @janbartel is that OK?

@janbartel
Copy link
Contributor Author

I'm ok with deprecation if it is entirely removed in jetty-10 and above.

@lachlan-roberts
Copy link
Contributor

ContainerInitializer.ServletContainerInitializerServletContextListener is a public class so we cant just remove it. And we have already done a release of jetty-10, so I think the best thing we can do is deprecate it right now.

@lachlan-roberts
Copy link
Contributor

This has been fixed with PR #5959.

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

No branches or pull requests

4 participants