From a5ad2b33ab9cf443efb52c9b36562c2100616b53 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Fri, 27 May 2016 11:26:32 +0100 Subject: [PATCH] Enable JNDI lookups during app context refresh without changing TCCL When Tomcat is starting up and JNDI is enabled, it binds the web app class loader into its ContextBindings, thereby enabling JNDI lookups on any thread that uses the web app class loader as its thread context class loader. When Boot starts an application, the application context is refreshed on the main thread which has the app class loader as its TCCL. This meant that any JNDI lookups performed during refresh would fail. gh-2038 described this problem and a fix was made in ff99bb0. The fix was to set the main thread's TCCL to be Tomcat's web app class loader. This fixed the JNDI lookup problem, but it has become apparent that it has caused other problems when testing an application. The fix for gh-2038 sets the main thread's TCCL when embedded Tomcat starts (during application context refresh) and then restores it when embedded Tomcat stops (as a result of the application context being closed). This causes problems during testing as, when application context's are cached, the close is delayed. This means that the main thread's TCCL isn't restored, causing subsequent tests to run with the wrong TCCL. This commit takes a different approach to fixing gh-2038. Rather than changing the main thread's TCCL, it binds the app class loader into Tomcat's ContextBindings, thereby enabling JNDI lookups from the main thread. To avoid leaving a reference to the app class loader in Tomcat's ContextBindings, it unbinds the app class loader at the end of application context refresh. This narrows the scope of the fix so that it only applies during application context refresh which is the period in which JNDI lookups were problematic. Note that the original fix could have been modified to restore the TCCL once context refresh has completed rather than waiting for the context to be closed. However, my feeling is that leaving the TCCL unchanged and specifically addressing the JNDI problem by manipulating the context bindings is a more precise, and hopefully safer, solution. Closes gh-6053 --- .../TomcatEmbeddedServletContainer.java | 18 ++++++++-- ...tEmbeddedServletContainerFactoryTests.java | 33 ++++++++++++++++--- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainer.java b/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainer.java index 90810cb4a95..2f60bcba54b 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainer.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainer.java @@ -20,6 +20,8 @@ import java.util.HashMap; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; +import javax.naming.NamingException; + import org.apache.catalina.Container; import org.apache.catalina.Context; import org.apache.catalina.Engine; @@ -30,6 +32,7 @@ import org.apache.catalina.connector.Connector; import org.apache.catalina.startup.Tomcat; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.apache.naming.ContextBindings; import org.springframework.boot.context.embedded.EmbeddedServletContainer; import org.springframework.boot.context.embedded.EmbeddedServletContainerException; @@ -93,8 +96,14 @@ public class TomcatEmbeddedServletContainer implements EmbeddedServletContainer // We can re-throw failure exception directly in the main thread rethrowDeferredStartupExceptions(); - ClassLoader classLoader = findContext().getLoader().getClassLoader(); - Thread.currentThread().setContextClassLoader(classLoader); + Context context = findContext(); + try { + ContextBindings.bindClassLoader(context, context.getNamingToken(), + getClass().getClassLoader()); + } + catch (NamingException ex) { + // Naming is not enabled. Continue + } // Unlike Jetty, all Tomcat threads are daemon threads. We create a // blocking non-daemon to stop immediate shutdown @@ -180,6 +189,11 @@ public class TomcatEmbeddedServletContainer implements EmbeddedServletContainer throw new EmbeddedServletContainerException( "Unable to start embedded Tomcat servlet container", ex); } + finally { + Context context = findContext(); + ContextBindings.unbindClassLoader(context, context.getNamingToken(), + getClass().getClassLoader()); + } } private void checkThatConnectorsHaveStarted() { diff --git a/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java b/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java index c29b7a055b1..2b57c02b096 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java @@ -24,6 +24,9 @@ import java.util.HashMap; import java.util.Map; import java.util.concurrent.TimeUnit; +import javax.naming.InitialContext; +import javax.naming.NamingException; + import org.apache.catalina.Container; import org.apache.catalina.Context; import org.apache.catalina.LifecycleEvent; @@ -399,14 +402,34 @@ public class TomcatEmbeddedServletContainerFactoryTests } @Test - public void tcclOfMainThreadIsTomcatWebAppClassLoader() { + public void jndiLookupsCanBePerformedDuringApplicationContextRefresh() + throws NamingException { Thread.currentThread().setContextClassLoader(getClass().getClassLoader()); - TomcatEmbeddedServletContainerFactory factory = getFactory(); + TomcatEmbeddedServletContainerFactory factory = new TomcatEmbeddedServletContainerFactory( + 0) { + + @Override + protected TomcatEmbeddedServletContainer getTomcatEmbeddedServletContainer( + Tomcat tomcat) { + tomcat.enableNaming(); + return super.getTomcatEmbeddedServletContainer(tomcat); + } + + }; + + // Container is created in onRefresh this.container = factory.getEmbeddedServletContainer(); + + // Lookups should now be possible + new InitialContext().lookup("java:comp/env"); + + // Called in finishRefresh, giving us an opportunity to remove the context binding + // and avoid a leak this.container.start(); - assertThat(Thread.currentThread().getContextClassLoader()) - .isInstanceOf(TomcatEmbeddedWebappClassLoader.class); - this.container.stop(); + + // Lookups should no longer be possible + this.thrown.expect(NamingException.class); + new InitialContext().lookup("java:comp/env"); } @Override