Clarify ServletContextListener.contextDestroyed() call requirements on application startup failure

[glassfishrobot]

When migrating an application from Jboss EAP 6 (Tomcat based) to WildFly, I've encountered a change of behavior with ServletContextListeners that are not cleaned-up properly anymore (contextDestroyed() called) if an exception is thrown during application startup.

Steps to reproduce:
1 - Register two listeners (in web.xml)
2 - First one initializes correctly (contextInitialized())
3 - Second one throws a RuntimeException (contextInitialized())
4 - (On Jboss EAP 6): contextDestroyed() was called on both listeners.
5 - Failed application was undeployed successfully.

Behavior on different servlet implementations:

• Tomcat (up to 9): contextDestroyed() is called on all listeners (including the one that failed to initialize)
• Jboss EAP 6: Same as Tomcat (expected)
• WildFly 8-10: contextDestroyed() not called on any listener.
• Glassfish 4: contextDestroyed() not called on any listener

The 2.5 and 3.0+ servlet-api documents are not clear on the expected behavior.

• 2.5, SRV.9.12 Web Application Deployment
  • Explains that contextInitialized() must be called for all instanciated listeners. No clear behavior for contextDestroyed().
• 3.0+, 8.2.3 Assembling the descriptor from web.xml, webfragment.xml
  and annotations.
  • Clarify the order of initialization and cleanup of listeners (based on order in web.xml).

In my opinion, the behavior I would expect is the following:

• contextDestroyed() SHOULD be called for every listeners for which a successful initialization occured. This way, proper cleanup would be ensured.
• contextDestroyed() SHOULD NOT be called for listeners that failed to initialized. Behaviour would be consistent with the "2.3.2.1 Error Conditions on Initialization" section on servlet initialization/cleanup.

[glassfishrobot]

@glassfishrobot Commented
Reported by glavoie84

[glassfishrobot]

@glassfishrobot Commented
glavoie84 said:
Here's a small code sample to reproduce.

import javax.servlet.ServletContextEvent;
import javax.servlet.ServletContextListener;

public class Listener1 implements ServletContextListener {
    @Override
    public void contextInitialized(ServletContextEvent servletContextEvent) {
        System.out.println("Listener1 initialized.");
    }

    @Override
    public void contextDestroyed(ServletContextEvent servletContextEvent) {
        System.out.println("Listener1 destroyed.");
    }
}

import javax.servlet.ServletContextEvent;
import javax.servlet.ServletContextListener;

public class Listener2 implements ServletContextListener {
    @Override
    public void contextInitialized(ServletContextEvent servletContextEvent) {
        System.out.println("Listener2 initialized.");
        throw new RuntimeException("Listener2 broken");
    }

    @Override
    public void contextDestroyed(ServletContextEvent servletContextEvent) {
        System.out.println("Listener2 destroyed.");
    }
}

<?xml version="1.0" encoding="UTF-8"?>
<web-app xmlns="http://xmlns.jcp.org/xml/ns/javaee"
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://xmlns.jcp.org/xml/ns/javaee http://xmlns.jcp.org/xml/ns/javaee/web-app_3_1.xsd"
         version="3.1">

    <listener>
        <listener-class>Listener1</listener-class>
    </listener>

    <listener>
        <listener-class>Listener2</listener-class>
    </listener>
</web-app>

Output on Tomcat/Jboss EAP 6:

Listener1 initialized.
Listener2 initialized.
... Stacktrace for RuntimeException("Listener2 broken")
Listener2 destroyed.
Listener1 destroyed.
... Container deployment error message.

Output on WildFly/Glassfish:

Listener1 initialized.
Listener2 initialized.
... Stacktrace for RuntimeException("Listener2 broken")
... Container deployment error message.

[glassfishrobot]

@glassfishrobot Commented
gregwilkins said:
For completeness Jetty produces the following results:

Listener1 initialized.
Listener2 initialized.
java.lang.RuntimeException: Listener2 broken
Listener2 destroyed.
Listener1 destroyed.

For further investigation, I added a third listener to see if it's initialised method get's called even after listener2 throws. Jetty's results are:

Listener1 initialized.
Listener2 initialized.
java.lang.RuntimeException: Listener2 broken
Listener3 destroyed.
Listener2 destroyed.
Listener1 destroyed.

Which is probably not good. Either Listener3 should be initialised or it's destroy should not be called.

I believe the OP is suggesting the results should be:

Listener1 initialized.
Listener2 initialized.
java.lang.RuntimeException: Listener2 broken
Listener1 destroyed.

This would be good to have clarified

[glassfishrobot]

@glassfishrobot Commented
glavoie84 said:
Greg,
thanks for adding Jetty's result. I also believe Jetty's result is not good with a 3rd listener. Your last sample output is exactly what I am suggesting.

Thanks!

[glassfishrobot]

@glassfishrobot Commented
gregwilkins said:
So what if Listener2 throws in destroy rather than initialized? Should Listener1 destroy still be called?

[glassfishrobot]

@glassfishrobot Commented
glavoie84 said:
I think that Listener1 destroy() should still be called as it was previously fully initialized, thus in a proper state for being destroyed properly.

Now given that we have the proper tools to do a proper cleanup, I think the developer has some responsibility to do some error management when required.

For example, let say Listener2 does a multi-step initialization that could possibly leak resources, but one of the step fails with an unmanageable error. It should be up to Listener2 to attempt to do as much cleanup as possible, still within the "contextInitialized()" call, before rethrowing the error. At this point the startup of the application would still fail, but previously initialized ServletContextListeners would still have a chance to do their cleanup. And other applications living on the container will have a chance to survive without a restart of the JVM.

Same if Listener2 fails during the contextDestroyed() call. We can't tell the outcome for all error scenarios, but other listeners would have the chance to do a proper cleanup and it becomes the developer's responsibility to tell if the error would require a container restart or not.

[glassfishrobot]

@glassfishrobot Commented
gregwilkins said:
So this is a jetty unit test that I believe describes what we think should happen:

ServletContextHandler context = new ServletContextHandler();

        final List<String> history=new ArrayList<>();

        context.addEventListener(new ServletContextListener()
        {
            @Override
            public void contextInitialized(ServletContextEvent servletContextEvent)
            {
history.add("I0");
            }

            @Override
            public void contextDestroyed(ServletContextEvent servletContextEvent)
            {
history.add("D0");
            }
        });
        context.addEventListener(new ServletContextListener()
        {
            @Override
            public void contextInitialized(ServletContextEvent servletContextEvent)
            {
history.add("I1");
            }

            @Override
            public void contextDestroyed(ServletContextEvent servletContextEvent)
            {
history.add("D1");
throw new RuntimeException("Listener1 destroy broken");
            }
        });
        context.addEventListener(new ServletContextListener()
        {
            @Override
            public void contextInitialized(ServletContextEvent servletContextEvent)
            {
history.add("I2");
throw new RuntimeException("Listener2 init broken");
            }

            @Override
            public void contextDestroyed(ServletContextEvent servletContextEvent)
            {
history.add("D2");
            }
        });
        context.addEventListener(new ServletContextListener()
        {
            @Override
            public void contextInitialized(ServletContextEvent servletContextEvent)
            {
history.add("I3");
            }

            @Override
            public void contextDestroyed(ServletContextEvent servletContextEvent)
            {
history.add("D3");
            }
        });

        try
        {
            context.start();
        }
        catch(Exception e)
        {
            history.add(e.getMessage());
        }
        finally
        {
            try
            {
context.stop();
            }
            catch(Exception e)
            {
while(e.getCause() instanceof Exception)
    e=(Exception)e.getCause();
history.add(e.getMessage());
            }
            finally
            {
            }
        }

        Assert.assertThat(history,Matchers.contains("I0","I1","I2","Listener2 init broken","D1","D0","Listener1 destroy broken"));

[glassfishrobot]

@glassfishrobot Commented
glavoie84 said:
I have a few comments on the unit test:

• The context.start() wrapping looks fine as the process should stop with the first exception.
• Not sure about the context.stop() implementation, but the wrapping suggests a single exception will be thrown, which would be the "Listener1 destroy broken" exception because it is added to history.
  • I see two options:
    • contextDestroyed() exceptions could be accumulated and returned by context.stop()
    • contextDestroyed() exceptions could be printed immediately as they occur and context.stop() would throw a more general exception to tell one or more issues occurred during the process.
• Matchers.contains() tells me that the order is not important in the history List, but it is (per servlet 3.0 spec).
  • Asserting the number of elements in the history List would also be a good idea.
  • With the two suggested error management scenarios, the "Listener1 destroy broken" entry would either stay at the end or appear right after "D1".

[glassfishrobot]

@glassfishrobot Commented
gregwilkins said:
I think the throwing of the exception is a little bit container specific. In jetty's case if there is only 1 exception it is thrown. If multiple exceptions are thrown then a MultiException is thrown with the first of the real exceptions as the cause and the rest are added as suppressed exceptions. In jetty behaviour is also related to how it was started (ie should the context stay up in unavailable state or should the whole container fail to start).

The important thing here is exactly which destroys get called?

[glassfishrobot]

@glassfishrobot Commented
glavoie84 said:
I agree with you that the exception management is container specific and I think that what you explain is perfectly fine. I just wasn't sure about how you wanted to test it with your unit test.

Yes, the important thing is which destroys get called.

[glassfishrobot]

@glassfishrobot Commented
This issue was imported from java.net JIRA SERVLET_SPEC-152

[glassfishrobot]

• Issue Imported From: https://github.com/javaee/servlet-spec/issues/152
• Original Issue Raised By:@glassfishrobot
• Original Issue Assigned To: @glassfishrobot