Refactor SpringBootServletInitializer.onStartup to initialize WebApplicationContext inside ServletContextListener

[apogrebnyak]

Currently, onStartup creates a web application, and only then registers a ServletContextListener (to react to contextDestroyed event).

If an application wants to register some other ServletContextListener before the SpringApplication is created it cannot reuse current onStartup method. I propose that onStartup be refactored to register a fully functioning ServletContextListener, that will be called by the Servlet container.

Here is a proposed refactoring:

@Override
public void onStartup (
        final ServletContext servletContext
    )
{
    servletContext.addListener(
        new ServletContextListener( ) {
            private ContextLoaderListener delegate = null;

            @Override
            public void contextInitialized ( final ServletContextEvent sce ) {
                final WebApplicationContext rootAppContext =
                    createRootApplicationContext(servletContext);

                if (rootAppContext != null) {
                    delegate = new ContextLoaderListener(rootAppContext);
                    // Do not call delegate.contextInitialized as we've 
                    // already initilized it
                }
                else {
                    this.logger.debug(
                        "No ContextLoaderListener registered, as "
                            + "createRootApplicationContext() did not "
                            + "return an application context");
                }
            }

            @Override
            public void contextDestroyed ( final ServletContextEvent sce ) {
                if ( delegate != null ) {
                    // Delegate call
                    delegate.contextDestroyed( sce );
                }
            }
        }
    );
}

With this change client application would be able to do the following:

public class ClientApp extends SpringBootServletInitializer {
  @Override
  public void onStartup (
        final ServletContext servletContext
    )
  {
    servletContext.addListener( new MyPreListener( ) );
    super.onStartup( servletContext );
    servletContext.addListener( new MyPostListener( ) );
  }
}

and ensure that listeners are called in order. Currently all listeners will be called only after SpringApplication has already been initialized (For some legacy applications this may not be desirable).

[philwebb]

@dsyer any thoughts?

[dsyer]

Looks harmless. (Famous last words.)

[apogrebnyak]

I have to be a devil's advocate on this. The only drawback that I can think of, is that if anybody had implemented ClientApp in a way I described originally, the MyPreListener will be called after the ApplicationContext is created. On the other hand, it would be destroyed after the ApplicationContext is destroyed (basically the ordering is not honored).

I don't know if anybody in the wild has any dependency on this behavior.

On the pro side, I think the proposed change is more in-line with the Servlet framework, as it does extra initialization through a well defined hook, and allows other cooperative players to rely on a defined call order.

-Alex

[philwebb]

I think we'll risk the change.

[philwebb]

[INFO] [talledLocalContainer] Caused by: java.lang.UnsupportedOperationException: Section 4.4 of the Servlet 3.0 specification does not permit this method to be called from a ServletContextListener that was not defined in web.xml, a web-fragment.xml file nor annotated with @WebListener
[INFO] [talledLocalContainer]   at org.apache.catalina.core.StandardContext$NoPluggabilityServletContext.addServlet(StandardContext.java:6942)
[INFO] [talledLocalContainer]   at org.springframework.boot.context.embedded.ServletRegistrationBean.onStartup(ServletRegistrationBean.java:166)
[INFO] [talledLocalContainer]   at org.springframework.boot.context.embedded.EmbeddedWebApplicationContext$1.onStartup(EmbeddedWebApplicationContext.java:203)
[INFO] [talledLocalContainer]   at org.springframework.boot.context.embedded.EmbeddedWebApplicationContext.createEmbeddedServletContainer(EmbeddedWebApplicationContext.java:153)
[INFO] [talledLocalContainer]   at org.springframework.boot.context.embedded.EmbeddedWebApplicationContext.onRefresh(EmbeddedWebApplicationContext.java:121)
[INFO] [talledLocalContainer]   ... 20 more
[INFO] [talledLocalContainer] 

[philwebb]

OK, I've needed to revert that because we can't add Servlets from the ServletContextListener.

[philwebb]

@apogrebnyak Can you provide a bit more detail about the underlying reason that you wanted this change. Perhaps there is some other way that we can fix it?

[apogrebnyak]

I've reviewed your checking and now I see why I did not hit this exception earlier.

In the original proposal, the listener is created through anonymous class, and the ServletContext it uses is bound to the onStartup method's parameter.

In the 1763148 a stand-alone listener class is created and the context passed in the event is used.

After debugging Tomcat initialization code I see that the two ServletContexts are not created equals, and one in the event is significantly neutered facade (I guess Tomcat needs it to be this way in order to initialize application runtime listeners properly).

Now back to my reason for requesting this bug. I had a legacy ServletContextListener, that I wanted to initialize before the SpringBoot application.

I think, in view of how fragile ServletContextListener is, this proposition of rewriting onStartup method is out, however, looks like the original intent (of registering another ServletContextListener before hitting SpringBootServletInitializer.onStartup) can be achived by implementing Ordered interface in SpringBootServletInitializer, and giving it a neutral precedence of 0.

So, please revert to original onStartup implementation and I am going to create a new issue and paste a reference here

[philwebb]

Great thanks. If you can provide a sample project with the new issue that you open that will be very helpful.

[apogrebnyak]

Created request for SpringBootServletInitializer.Ordered implementation ->
#2098