Defer removal of Connectors until after ServletContext initialization

Previously, we removed the Connectors from Tomcat's Service before
the Context was started. The removal of the Connectors is required as
it prevents Tomcat from accepting requests before we're ready to
handle them.

Part of starting the Context is creating and initializing the
ServletContext. ServerProperties uses a ServletContextInitializer to
set the session tracking modes and Tomcat rejects the SSL tracking
mode if there is no SSL-enabled connector available. With the previous
arrangement this led to a failure as the Connectors had been removed
so the SSL-enabled connector could not be found.

This commit updates the embedded Tomcat container to defer the
removal of the Connectors until after the context has been started
but still at a point that is before the Connectors themselves would
have been started.

Closes gh-12058

Author: wilkinsona

spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/ServerPropertiesTests.java

@@ -23,6 +23,7 @@
 import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
 
 import javax.servlet.ServletContext;
 import javax.servlet.ServletException;
@@ -43,6 +44,7 @@
 
 import org.springframework.beans.MutablePropertyValues;
 import org.springframework.boot.bind.RelaxedDataBinder;
+import org.springframework.boot.context.embedded.AbstractEmbeddedServletContainerFactory;
 import org.springframework.boot.context.embedded.ConfigurableEmbeddedServletContainer;
 import org.springframework.boot.context.embedded.EmbeddedServletContainer;
 import org.springframework.boot.context.embedded.jetty.JettyEmbeddedServletContainerFactory;
@@ -53,9 +55,7 @@
 import org.springframework.mock.env.MockEnvironment;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.BDDMockito.given;
 import static org.mockito.Matchers.anyBoolean;
-import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
@@ -314,7 +314,22 @@ public void testCustomizeDisplayName() throws Exception {
 	}
 
 	@Test
-	public void customizeSessionProperties() throws Exception {
+	public void customizeSessionPropertiesWithJetty() throws Exception {
+		customizeSessionProperties(new TomcatEmbeddedServletContainerFactory(0));
+	}
+
+	@Test
+	public void customizeSessionPropertiesWithTomcat() throws Exception {
+		customizeSessionProperties(new TomcatEmbeddedServletContainerFactory(0));
+	}
+
+	@Test
+	public void customizeSessionPropertiesWithUndertow() throws Exception {
+		customizeSessionProperties(new UndertowEmbeddedServletContainerFactory(0));
+	}
+
+	private void customizeSessionProperties(
+			AbstractEmbeddedServletContainerFactory factory) {
 		Map<String, String> map = new HashMap<String, String>();
 		map.put("server.session.timeout", "123");
 		map.put("server.session.tracking-modes", "cookie,url");
@@ -326,38 +341,80 @@ public void customizeSessionProperties() throws Exception {
 		map.put("server.session.cookie.secure", "true");
 		map.put("server.session.cookie.max-age", "60");
 		bindProperties(map);
-		ConfigurableEmbeddedServletContainer factory = mock(
-				ConfigurableEmbeddedServletContainer.class);
-		ServletContext servletContext = mock(ServletContext.class);
-		SessionCookieConfig sessionCookieConfig = mock(SessionCookieConfig.class);
-		given(servletContext.getSessionCookieConfig()).willReturn(sessionCookieConfig);
 		this.properties.customize(factory);
-		triggerInitializers(factory, servletContext);
-		verify(factory).setSessionTimeout(123);
-		verify(servletContext).setSessionTrackingModes(
-				EnumSet.of(SessionTrackingMode.COOKIE, SessionTrackingMode.URL));
-		verify(sessionCookieConfig).setName("testname");
-		verify(sessionCookieConfig).setDomain("testdomain");
-		verify(sessionCookieConfig).setPath("/testpath");
-		verify(sessionCookieConfig).setComment("testcomment");
-		verify(sessionCookieConfig).setHttpOnly(true);
-		verify(sessionCookieConfig).setSecure(true);
-		verify(sessionCookieConfig).setMaxAge(60);
-	}
-
-	private void triggerInitializers(ConfigurableEmbeddedServletContainer container,
-			ServletContext servletContext) throws ServletException {
-		verify(container, atLeastOnce())
-				.addInitializers(this.initializersCaptor.capture());
-		for (Object initializers : this.initializersCaptor.getAllValues()) {
-			if (initializers instanceof ServletContextInitializer) {
-				((ServletContextInitializer) initializers).onStartup(servletContext);
-			}
-			else {
-				for (ServletContextInitializer initializer : (ServletContextInitializer[]) initializers) {
-					initializer.onStartup(servletContext);
-				}
-			}
+		final AtomicReference<ServletContext> servletContextReference = new AtomicReference<ServletContext>();
+		EmbeddedServletContainer container = factory
+				.getEmbeddedServletContainer(new ServletContextInitializer() {
+
+					@Override
+					public void onStartup(ServletContext servletContext)
+							throws ServletException {
+						servletContextReference.set(servletContext);
+					}
+
+				});
+		try {
+			container.start();
+			SessionCookieConfig sessionCookieConfig = servletContextReference.get()
+					.getSessionCookieConfig();
+			assertThat(factory.getSessionTimeout()).isEqualTo(123);
+			assertThat(sessionCookieConfig.getName()).isEqualTo("testname");
+			assertThat(sessionCookieConfig.getDomain()).isEqualTo("testdomain");
+			assertThat(sessionCookieConfig.getPath()).isEqualTo("/testpath");
+			assertThat(sessionCookieConfig.getComment()).isEqualTo("testcomment");
+			assertThat(sessionCookieConfig.isHttpOnly()).isTrue();
+			assertThat(sessionCookieConfig.isSecure()).isTrue();
+			assertThat(sessionCookieConfig.getMaxAge()).isEqualTo(60);
+			assertThat(servletContextReference.get().getEffectiveSessionTrackingModes())
+					.isEqualTo(EnumSet.of(SessionTrackingMode.COOKIE,
+							SessionTrackingMode.URL));
+		}
+		finally {
+			container.stop();
+		}
+	}
+
+	@Test
+	public void sslSessionTrackingWithJetty() throws Exception {
+		sslSessionTracking(new JettyEmbeddedServletContainerFactory(0));
+	}
+
+	@Test
+	public void sslSessionTrackingWithTomcat() throws Exception {
+		sslSessionTracking(new TomcatEmbeddedServletContainerFactory(0));
+	}
+
+	@Test
+	public void sslSessionTrackingWithUndertow() throws Exception {
+		sslSessionTracking(new UndertowEmbeddedServletContainerFactory(0));
+	}
+
+	private void sslSessionTracking(AbstractEmbeddedServletContainerFactory factory) {
+		Map<String, String> map = new HashMap<String, String>();
+		map.put("server.ssl.enabled", "true");
+		map.put("server.ssl.key-store", "src/test/resources/test.jks");
+		map.put("server.ssl.key-password", "password");
+		map.put("server.session.tracking-modes", "ssl");
+		bindProperties(map);
+		this.properties.customize(factory);
+		final AtomicReference<ServletContext> servletContextReference = new AtomicReference<ServletContext>();
+		EmbeddedServletContainer container = factory
+				.getEmbeddedServletContainer(new ServletContextInitializer() {
+
+					@Override
+					public void onStartup(ServletContext servletContext)
+							throws ServletException {
+						servletContextReference.set(servletContext);
+					}
+
+				});
+		try {
+			container.start();
+			assertThat(servletContextReference.get().getEffectiveSessionTrackingModes())
+					.isEqualTo(EnumSet.of(SessionTrackingMode.SSL));
+		}
+		finally {
+			container.stop();
 		}
 	}
 

spring-boot-autoconfigure/src/test/resources/test.jks

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2017 the original author or authors.
+ * Copyright 2012-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -25,7 +25,10 @@
 import org.apache.catalina.Container;
 import org.apache.catalina.Context;
 import org.apache.catalina.Engine;
+import org.apache.catalina.Lifecycle;
+import org.apache.catalina.LifecycleEvent;
 import org.apache.catalina.LifecycleException;
+import org.apache.catalina.LifecycleListener;
 import org.apache.catalina.LifecycleState;
 import org.apache.catalina.Service;
 import org.apache.catalina.connector.Connector;
@@ -91,17 +94,28 @@ private void initialize() throws EmbeddedServletContainerException {
 			try {
 				addInstanceIdToEngineName();
 				try {
-					// Remove service connectors to that protocol binding doesn't happen
-					// yet
-					removeServiceConnectors();
+					final Context context = findContext();
+					context.addLifecycleListener(new LifecycleListener() {
+
+						@Override
+						public void lifecycleEvent(LifecycleEvent event) {
+							if (context.equals(event.getSource())
+									&& Lifecycle.START_EVENT.equals(event.getType())) {
+								// Remove service connectors so that protocol
+								// binding doesn't happen when the service is
+								// started.
+								removeServiceConnectors();
+							}
+						}
+
+					});
 
 					// Start the server to trigger initialization listeners
 					this.tomcat.start();
 
 					// We can re-throw failure exception directly in the main thread
 					rethrowDeferredStartupExceptions();
 
-					Context context = findContext();
 					try {
 						ContextBindings.bindClassLoader(context, getNamingToken(context),
 								getClass().getClassLoader());