-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Programmatically registered ServletContextListener is not called in 9.4.14 #3137
Comments
@wilkinsona it was actually a bug that it was working in 9.4.13 (and probably before that too) :) There was an error in the ServletContextHandler.Context whereby when addListener(Listener) was called, it was incorrectly added directly to the listener list AND also added to the list of ListenerHolders. It should only have ever been added to the list of ListenerHolders. See commit 1845e6e. All the ListenerHolders are converted to the actual Listener class before the webapp's beans are started, thus from 9.4.14 onwards its not possible to add a listener with a webapp bean. Can you explain your use case and I'll try and suggest another way of achieving it? |
Thanks for the background. The attached Maven project is a minimal recreation of Spring Boot's embedded Jetty integration. We have a |
@wilkinsona so would it be possible to change to using a ServletContainerInitializer to do the additions of servlets/filters/listeners? You can set the context attribute "org.eclipse.jetty.containerInitializerOrder" to make sure your SCI is the first one called: use the name of the class (as returned by class.getName()), followed by "*" to match any other SCIs discovered. Eg "com.acme.FooInitializer, *". Would that be possible, or does this have to take place external to the servlet api (and if so, why?)? |
We deliberately do not support SCIs for the reasons explained here. If it's possible for us to register an SCI without also causing any other SCIs to be detected that might be an option, but a mechanism outside of the Servlet API like we've been using up until 9.4.14 would be preferable. |
@wilkinsona ok, I have something else for you to try: Instead of calling webappcontext.getServletContext().addListener(l), instead do webappcontext.addEventListener(l) instead: this will add the listener instance directly to the list of listeners and bypass the ListenerHolder etc. As you are now effectively managing the lifecycle of that listener yourself, you should also really organize to remove it when the context stops. So you could modify the TestServletContextListener to pass in the WebAppContext, and in the contextDestroyed() method, you can remove itself from the list of listeners with webapp.removeEventListeners(this). Let me know how that works out for you. |
Thanks for the suggestion. Unfortunately, an important detail has been lost in my attempt to provide an example that's as minimal as possible. There's a layer of indirection between the Failing that, wasn't the bug in 9.4.13 that's fixed by 1845e6e a regression? It looks like it fixes #3097 which apparently worked fine in 9.4.12 and earlier. Would it be possible to restore 9.4.12's behaviour so that the scenario described in #3097 works and Spring Boot continues to work as it has since Jetty 8.1 (at least)? |
Any news on this please? As things stand we're stuck on Jetty 9.4.12 which isn't viable in the long term. |
@wilkinsona sorry for the delay, I've been travelling. Here's a suggestion to try out: remove the ServletContextInitializerConfiguration.Initializer class and move the code from line 80 (https://github.com/spring-projects/spring-boot/blob/v2.0.6.RELEASE/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/ServletContextInitializerConfiguration.java#L80) up into the ServletContextInitializerConfiguration.configure() method at line 53 (https://github.com/spring-projects/spring-boot/blob/v2.0.6.RELEASE/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/ServletContextInitializerConfiguration.java#L53). I've done a quick test by hacking up a jetty Configuration that called webapp.getServletContext().addListener(foo) inside the Configuration.configure method and it seemed ok, but you'd need to test it in your environment to be sure. |
Thanks very much, @janbartel. That appears to work nicely. |
Problem solved. |
There appears to be a regression in 9.4.14 that means that a programmatically registered
ServletContextListener
is no longer called. The problem can be reproduced using theattached Maven project. It contains a single test.
It should pass with 9.4.13:
And fail with 9.4.14:
The text was updated successfully, but these errors were encountered: