Fix #5835 Durable Filters, Servlets and Listeners #5837
Conversation
Filters, Servlets and Listeners are only durable if added prior to starting.
For non durable ServletContextListeners ensure that initialized is called if they are added during starting and that destroyed is called.
Fixed destroy order.
| @@ -689,7 +689,7 @@ public void addEventListener(EventListener listener) | |||
| { | |||
| _eventListeners.add(listener); | |||
|
|
|||
| if (!(isStarted() || isStarting())) | |||
| if (!isRunning()) | |||
| { | |||
| _durableListeners.add(listener); | |||
There was a problem hiding this comment.
Is this _durableListeners dumped during server dump?
Either as the collection it is, or by decorating the normal listener collection with something indicating that it is a durable listener?
Seems like that would have been useful for debug of behaviors, especially considering that !isRunning() above is a subtle behavior that isn't documented in the apidoc.
There was a problem hiding this comment.
It would be good info to dump, but not easy to do without making things very verbose (ie repeating the listeners) as when dumping a listener it doesn't know if it is durable or not.
|
@lachlan-roberts can you look at the failing tests in this one, as it is the stop/start websocket test that this PR is meant to help with. |
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Update WebSocketUpgradeFilter to have durable configuration.
|
@janbartel this is ready for review now |
| @@ -131,12 +132,12 @@ public static WebSocketUpgradeFilter configureContext(ServletContext context) th | |||
|
|
|||
| private NativeWebSocketConfiguration configuration; | |||
| private String instanceKey; | |||
| private boolean localConfiguration = false; | |||
| private boolean durableConfiguration = false; | |||
There was a problem hiding this comment.
The no-args constructor could call this((NativeWebSocketConfiguration)null) and then this could be final.
| @@ -146,6 +147,7 @@ public WebSocketUpgradeFilter(WebSocketServerFactory factory) | |||
|
|
|||
| public WebSocketUpgradeFilter(NativeWebSocketConfiguration configuration) | |||
| { | |||
| durableConfiguration = configuration != null; | |||
There was a problem hiding this comment.
There is something strange about this.
There is currently no code which creates a WebSocketUpgradeFilter without a configuration except when the filter is added with the class.
context.addFilter(WebSocketUpgradeFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));Even when we have a non-durable filter we use a durable configuration, which turns out it doesn't matter as the filter is not reused.
So now we can have:
- durable filter with durable configuration
- durable filter with non-durable configuration
- non-durable filter with durable configuration
- non-durable filter with non-durable configuration
Do we really need all these cases, can't we work out if the filter is durable and make the configuration the same. Maybe we could do something like:
@Override
public void init(FilterConfig config) throws ServletException
{
ServletContextHandler servletContextHandler = ServletContextHandler.getServletContextHandler(config.getServletContext());
ServletHandler servletHandler = servletContextHandler.getServletHandler();
durableConfiguration = servletHandler.isDurable(this);
...
| @@ -702,7 +702,16 @@ public void addEventListener(EventListener listener) | |||
| } | |||
|
|
|||
| if (listener instanceof ServletContextListener) | |||
| _servletContextListeners.add((ServletContextListener)listener); | |||
| { | |||
There was a problem hiding this comment.
I think we should throw IllegalStateException here: you should not be able to add a ServletContextListener after the context has initialized, as this is contrary to the Servlet Spec. I know this is a jetty api, however if you're using a SCL, you're using the spec.
Furthermore, the ServletContextListener is supposed to be called before any servlets, filters etc have been initialized, whereas this change allows a ServletContextListener to be called at any old time maybe even in a shutdown sequence - who knows what could happen.
I also do not think it's a good idea to do the listener initialization in multiple places: having it in 1 place makes the (already very complicated) initialization sequence very clear.
There was a problem hiding this comment.
Will try with ISE and see if we break anything....
jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java
Show resolved
Hide resolved
throw ISE for SCL added after init
|
This PR has test failures |
|
This needs to be rebased onto Jetty-10 |
|
replaced by #6027 |
#5835 Filters, Servlets and Listeners are only durable if added prior to starting.