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.

Sign up for GitHub

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

Jump to bottom

Fix #5835 Durable Filters, Servlets and Listeners #5837

Closed
wants to merge 10 commits into from
Closed

Fix #5835 Durable Filters, Servlets and Listeners #5837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6a1bcea
Fix #5835 Durable Filters, Servlets and Listeners
gregw Dec 22, 2020
1fd8caf
Fix #5835 Durable Filters, Servlets and Listeners
gregw Dec 22, 2020
d72046c
Fix #5835 Durable Filters, Servlets and Listeners
gregw Dec 22, 2020
4b6930f
Issue #5835 - fix tests for restarting webapp with websocket enabled
lachlan-roberts Dec 23, 2020
176e58d
Fix #5835 Durable Filters, Servlets and Listeners
gregw Dec 23, 2020
b9f0e1b
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-58…
gregw Dec 29, 2020
ba34eb7
Fix #5835 Durable Filters, Servlets and Listeners
gregw Dec 29, 2020
4103173
Merge branch 'jetty-9.4.x' into jetty-9.4.x-5835-durable
gregw Feb 9, 2021
cc75e4e
minor cleanup
gregw Feb 9, 2021
72a6de8
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-58…
gregw Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 13 additions & 21 deletions jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java
View file
Open in desktop
Expand Up @@ -689,7 +689,7 @@ public void addEventListener(EventListener listener)
{
_eventListeners.add(listener);

if (!(isStarted() || isStarting()))
if (!isRunning())
{
_durableListeners.add(listener);
Copy link
Contributor

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.

Copy link
Contributor Author

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.

}
Expand All @@ -702,7 +702,11 @@ public void addEventListener(EventListener listener)
}

if (listener instanceof ServletContextListener)
{
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will try with ISE and see if we break anything....

if (_contextStatus == ContextStatus.INITIALIZED)
throw new IllegalStateException("Context is initialized");
_servletContextListeners.add((ServletContextListener)listener);
}

if (listener instanceof ServletContextAttributeListener)
_servletContextAttributeListeners.add((ServletContextAttributeListener)listener);
Expand Down Expand Up @@ -982,31 +986,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;
}
}

Expand Down
64 changes: 64 additions & 0 deletions jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.atomic.AtomicInteger;
import javax.servlet.ServletContextEvent;
import javax.servlet.ServletContextListener;
import javax.servlet.http.HttpServletRequest;
Expand Down Expand Up @@ -482,6 +483,69 @@ public void testContextInitializationDestruction() throws Exception
assertEquals(1, listener.destroyed);
}

@Test
public void testNonDurableContextListener() throws Exception
{
Server server = new Server();
ContextHandler context = new ContextHandler();
server.setHandler(context);
AtomicInteger initialized = new AtomicInteger();
AtomicInteger destroyed = new AtomicInteger();

context.addEventListener(new ServletContextListener()
{
@Override
public void contextInitialized(ServletContextEvent sce)
{
initialized.incrementAndGet();
context.addEventListener(new ServletContextListener()
{
@Override
public void contextInitialized(ServletContextEvent sce)
{
initialized.incrementAndGet();
}

@Override
public void contextDestroyed(ServletContextEvent sce)
{
destroyed.incrementAndGet();
}
});
}

@Override
public void contextDestroyed(ServletContextEvent sce)
{
destroyed.incrementAndGet();
}
});

server.start();
assertThat(initialized.get(), is(2));

context.addEventListener(new ServletContextListener()
{
@Override
public void contextInitialized(ServletContextEvent sce)
{
// This should not get called because added after started
initialized.incrementAndGet();
}

@Override
public void contextDestroyed(ServletContextEvent sce)
{
destroyed.incrementAndGet();
}
});

assertThat(initialized.get(), is(2));

server.stop();
assertThat(destroyed.get(), is(3));
}

@Test
public void testContextVirtualGetContext() throws Exception
{
Expand Down
122 changes: 57 additions & 65 deletions jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java
View file
Open in desktop
Expand Up @@ -26,7 +26,6 @@
import java.util.EventListener;
import java.util.HashMap;
import java.util.List;
import java.util.ListIterator;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
Expand Down Expand Up @@ -111,6 +110,7 @@ public class ServletHandler extends ScopedHandler
private List<FilterMapping> _filterPathMappings;
private MultiMap<FilterMapping> _filterNameMappings;
private List<FilterMapping> _wildFilterNameMappings;
private final List<BaseHolder<?>> _durable = new ArrayList<>();

private final Map<String, ServletHolder> _servletNameMap = new HashMap<>();
private PathMappings<ServletHolder> _servletPathMap;
Expand Down Expand Up @@ -160,6 +160,11 @@ protected synchronized void doStart()
_identityService = securityHandler.getIdentityService();
}

_durable.clear();
_durable.addAll(Arrays.asList(getFilters()));
_durable.addAll(Arrays.asList(getServlets()));
_durable.addAll(Arrays.asList(getListeners()));

updateNameMappings();
updateMappings();

Expand Down Expand Up @@ -232,37 +237,31 @@ protected synchronized void doStop()
// Stop filters
List<FilterHolder> filterHolders = new ArrayList<>();
List<FilterMapping> filterMappings = ArrayUtil.asMutableList(_filterMappings);
if (_filters != null)
for (int i = _filters == null ? 0 : _filters.length; i-- > 0;)
{
for (int i = _filters.length; i-- > 0; )
FilterHolder f = _filters[i];
try
{
try
{
_filters[i].stop();
}
catch (Exception e)
{
LOG.warn(Log.EXCEPTION, e);
}
if (_filters[i].getSource() != Source.EMBEDDED)
{
//remove all of the mappings that were for non-embedded filters
_filterNameMap.remove(_filters[i].getName());
//remove any mappings associated with this filter
ListIterator<FilterMapping> fmitor = filterMappings.listIterator();
while (fmitor.hasNext())
{
FilterMapping fm = fmitor.next();
if (fm.getFilterName().equals(_filters[i].getName()))
fmitor.remove();
}
}
else
filterHolders.add(_filters[i]); //only retain embedded
f.stop();
}
catch (Exception e)
{
LOG.warn(Log.EXCEPTION, e);
}
if (_durable.contains(f))
{
filterHolders.add(f); //only retain embedded
}
else
{
//remove all of the mappings that were for non-embedded filters
_filterNameMap.remove(f.getName());
//remove any mappings associated with this filter
filterMappings.removeIf(fm -> fm.getFilterName().equals(f.getName()));
}
}

//Retain only filters and mappings that were added using jetty api (ie Source.EMBEDDED)
//Retain only filters and mappings that were added using jetty api prior to starting
FilterHolder[] fhs = filterHolders.toArray(new FilterHolder[0]);
updateBeans(_filters, fhs);
_filters = fhs;
Expand All @@ -276,38 +275,32 @@ protected synchronized void doStop()
// Stop servlets
List<ServletHolder> servletHolders = new ArrayList<>(); //will be remaining servlets
List<ServletMapping> servletMappings = ArrayUtil.asMutableList(_servletMappings); //will be remaining mappings
if (_servlets != null)
for (int i = _servlets == null ? 0 : _servlets.length; i-- > 0;)
{
for (int i = _servlets.length; i-- > 0; )
ServletHolder s = _servlets[i];
try
{
try
{
_servlets[i].stop();
}
catch (Exception e)
{
LOG.warn(Log.EXCEPTION, e);
}
s.stop();
}
catch (Exception e)
{
LOG.warn(Log.EXCEPTION, e);
}

if (_servlets[i].getSource() != Source.EMBEDDED)
{
//remove from servlet name map
_servletNameMap.remove(_servlets[i].getName());
//remove any mappings associated with this servlet
ListIterator<ServletMapping> smitor = servletMappings.listIterator();
while (smitor.hasNext())
{
ServletMapping sm = smitor.next();
if (sm.getServletName().equals(_servlets[i].getName()))
smitor.remove();
}
}
else
servletHolders.add(_servlets[i]); //only retain embedded
if (_durable.contains(s))
{
servletHolders.add(s); //only retain embedded
}
else
{
//remove from servlet name map
_servletNameMap.remove(s.getName());
//remove any mappings associated with this servlet
servletMappings.removeIf(sm -> sm.getServletName().equals(s.getName()));
}
}

//Retain only Servlets and mappings added via jetty apis (ie Source.EMBEDDED)
// Retain only Servlets and mappings added via jetty apis prior to starting
ServletHolder[] shs = servletHolders.toArray(new ServletHolder[0]);
updateBeans(_servlets, shs);
_servlets = shs;
Expand All @@ -320,22 +313,21 @@ protected synchronized void doStop()

//Retain only Listeners added via jetty apis (is Source.EMBEDDED)
List<ListenerHolder> listenerHolders = new ArrayList<>();
if (_listeners != null)
for (int i = _listeners == null ? 0 : _listeners.length; i-- > 0;)
{
for (int i = _listeners.length; i-- > 0; )
ListenerHolder l = _listeners[i];
try
{
try
{
_listeners[i].stop();
}
catch (Exception e)
{
LOG.warn(Log.EXCEPTION, e);
}
if (_listeners[i].getSource() == Source.EMBEDDED)
listenerHolders.add(_listeners[i]);
l.stop();
}
catch (Exception e)
{
LOG.warn(Log.EXCEPTION, e);
}
if (_durable.contains(l))
listenerHolders.add(l);
}

ListenerHolder[] listeners = listenerHolders.toArray(new ListenerHolder[0]);
updateBeans(_listeners, listeners);
_listeners = listeners;
Expand Down
2 changes: 2 additions & 0 deletions jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java
View file
Open in desktop
Expand Up @@ -1464,11 +1464,13 @@ public void testFiltered() throws Exception
Path image = docRoot.resolve("image.jpg");
createFile(image, "not an image");

server.stop();
ServletHolder defholder = context.addServlet(DefaultServlet.class, "/");
defholder.setInitParameter("dirAllowed", "false");
defholder.setInitParameter("redirectWelcome", "false");
defholder.setInitParameter("welcomeServlets", "false");
defholder.setInitParameter("gzip", "false");
server.start();

String rawResponse;
HttpTester.Response response;
Expand Down