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
Fix #5835 Durable Filters, Servlets and Listeners #5837
Changes from 5 commits
6a1bcea
1fd8caf
d72046c
4b6930f
176e58d
b9f0e1b
ba34eb7
4103173
cc75e4e
72a6de8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -689,7 +689,7 @@ public void addEventListener(EventListener listener) | |
{ | ||
_eventListeners.add(listener); | ||
|
||
if (!(isStarted() || isStarting())) | ||
if (!isRunning()) | ||
{ | ||
_durableListeners.add(listener); | ||
} | ||
|
@@ -702,7 +702,16 @@ public void addEventListener(EventListener listener) | |
} | ||
|
||
if (listener instanceof ServletContextListener) | ||
_servletContextListeners.add((ServletContextListener)listener); | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will try with ISE and see if we break anything.... |
||
ServletContextListener scl = (ServletContextListener)listener; | ||
_servletContextListeners.add(scl); | ||
if (_contextStatus == ContextStatus.INITIALIZED) | ||
{ | ||
if (isStarting()) | ||
scl.contextInitialized(new ServletContextEvent(_scontext)); | ||
_destroyServletContextListeners.add(scl); | ||
} | ||
} | ||
|
||
if (listener instanceof ServletContextAttributeListener) | ||
_servletContextAttributeListeners.add((ServletContextAttributeListener)listener); | ||
|
@@ -982,31 +991,19 @@ protected void startContext() throws Exception | |
public void contextInitialized() throws Exception | ||
{ | ||
// Call context listeners | ||
switch (_contextStatus) | ||
if (_contextStatus == ContextStatus.NOTSET) | ||
{ | ||
case NOTSET: | ||
_contextStatus = ContextStatus.INITIALIZED; | ||
gregw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_destroyServletContextListeners.clear(); | ||
if (!_servletContextListeners.isEmpty()) | ||
{ | ||
try | ||
{ | ||
_destroyServletContextListeners.clear(); | ||
if (!_servletContextListeners.isEmpty()) | ||
{ | ||
ServletContextEvent event = new ServletContextEvent(_scontext); | ||
for (ServletContextListener listener : _servletContextListeners) | ||
{ | ||
callContextInitialized(listener, event); | ||
_destroyServletContextListeners.add(listener); | ||
} | ||
} | ||
} | ||
finally | ||
ServletContextEvent event = new ServletContextEvent(_scontext); | ||
for (ServletContextListener listener : _servletContextListeners) | ||
{ | ||
_contextStatus = ContextStatus.INITIALIZED; | ||
callContextInitialized(listener, event); | ||
_destroyServletContextListeners.add(listener); | ||
} | ||
break; | ||
} | ||
default: | ||
break; | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.