SLING-8187 - Deadlock in SlingMainServlet after SLING-8051
authorJulian Sedding <jsedding@apache.org>
Thu, 20 Dec 2018 13:05:49 +0000 (14:05 +0100)
committerJulian Sedding <jsedding@apache.org>
Mon, 7 Jan 2019 12:34:19 +0000 (13:34 +0100)
- initialize and register SlingServletContext and all dependent objects asynchronously
- make SlingServletContext#dispose a no-op on subsequent calls
- await completion of async initialization in SlingMainServlet#service() method
- await completion of async initialization in #deactivate() in order to simplify clean shutdown

src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java
src/main/java/org/apache/sling/engine/impl/helper/SlingServletContext.java

index 458441e..e9cd272 100644 (file)
@@ -24,6 +24,8 @@ import java.util.Enumeration;
 import java.util.Hashtable;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 import java.util.regex.Pattern;
 
 import javax.servlet.GenericServlet;
@@ -162,14 +164,14 @@ public class SlingMainServlet extends GenericServlet {
      * <code>ServletContext.getServerInfo()</code> method. This field defaults
      * to {@link #PRODUCT_NAME} and is amended with the major and minor version
      * of the Sling Engine bundle while this component is being
-     * {@link #activate(BundleContext, Map)} activated}.
+     * {@link #activate(BundleContext, Map, Config)} activated}.
      */
     private String productInfo = PRODUCT_NAME;
 
     /**
      * The server information to report in the {@link #getServerInfo()} method.
      * By default this is just the {@link #PRODUCT_NAME} (same as
-     * {@link #productInfo}. During {@link #activate(BundleContext, Map)}
+     * {@link #productInfo}. During {@link #activate(BundleContext, Map, Config)}
      * activation} the field is updated with the full {@link #productInfo} value
      * as well as the operating system and java version it is running on.
      * Finally during servlet initialization the product information from the
@@ -201,12 +203,18 @@ public class SlingMainServlet extends GenericServlet {
 
     private String configuredServerInfo;
 
+    private CountDownLatch asyncActivation = new CountDownLatch(1);
+
     // ---------- Servlet API -------------------------------------------------
 
     @Override
     public void service(ServletRequest req, ServletResponse res)
             throws ServletException {
 
+        if (!awaitQuietly(asyncActivation, 30)) {
+            throw new ServletException("Servlet not initialized after 30 seconds");
+        }
+
         if (req instanceof HttpServletRequest
             && res instanceof HttpServletResponse) {
 
@@ -273,6 +281,15 @@ public class SlingMainServlet extends GenericServlet {
 
     // ---------- Internal helper ----------------------------------------------
 
+    private static boolean awaitQuietly(CountDownLatch latch, int seconds) {
+        try {
+            return latch.await(seconds, TimeUnit.SECONDS);
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+        }
+        return false;
+    }
+
     /**
      * Sets the {@link #productInfo} field from the providing bundle's version
      * and the {@link #PRODUCT_NAME}.
@@ -438,57 +455,108 @@ public class SlingMainServlet extends GenericServlet {
             RequestHistoryConsolePlugin.initPlugin(bundleContext, maxRequests, compiledPatterns);
         } catch (Throwable t) {
             log.debug(
-                "Unable to register web console request recorder plugin.", t);
+                    "Unable to register web console request recorder plugin.", t);
         }
+    }
 
-        try {
-            Dictionary<String, String> mbeanProps = new Hashtable<>();
-            mbeanProps.put("jmx.objectname", "org.apache.sling:type=engine,service=RequestProcessor");
+    @Override
+    public void init() {
+        setServerInfo();
+        log.info("{} ready to serve requests", this.getServerInfo());
+        asyncSlingServletContextRegistration();
+    }
 
-            RequestProcessorMBeanImpl mbean = new RequestProcessorMBeanImpl();
-            requestProcessorMBeanRegistration = bundleContext.registerService(RequestProcessorMBean.class, mbean, mbeanProps);
-            requestProcessor.setMBean(mbean);
-        } catch (Throwable t) {
-            log.debug("Unable to register mbean");
-        }
+    // registration needs to be async. if it is done synchronously
+    // there is potential for a deadlock involving Felix global lock
+    // and a lock held by HTTP Whiteboard while calling Servlet#init()
+    private void asyncSlingServletContextRegistration() {
+        Thread thread = new Thread("SlingServletContext registration") {
+            @Override
+            public void run() {
+                try {
+                    // note: registration of SlingServletContext as a service is delayed to the #init() method
+                    slingServletContext = new SlingServletContext(bundleContext, SlingMainServlet.this);
+                    slingServletContext.register(bundleContext);
+
+                    // register render filters already registered after registration with
+                    // the HttpService as filter initialization may cause the servlet
+                    // context to be required (see SLING-42)
+                    filterManager = new ServletFilterManager(bundleContext,
+                                                                    slingServletContext);
+                    filterManager.open();
+                    requestProcessor.setFilterManager(filterManager);
+
+                    // initialize requestListenerManager
+                    requestListenerManager = new RequestListenerManager(bundleContext, slingServletContext);
+
+                    // Setup configuration printer
+                    printerRegistration = WebConsoleConfigPrinter.register(bundleContext, filterManager);
 
-        // provide the SlingRequestProcessor service
-        Hashtable<String, String> srpProps = new Hashtable<>();
-        srpProps.put(Constants.SERVICE_VENDOR, "The Apache Software Foundation");
-        srpProps.put(Constants.SERVICE_DESCRIPTION, "Sling Request Processor");
-        requestProcessorRegistration = bundleContext.registerService(
-            SlingRequestProcessor.class, requestProcessor, srpProps);
+                    try {
+                        Dictionary<String, String> mbeanProps = new Hashtable<>();
+                        mbeanProps.put("jmx.objectname", "org.apache.sling:type=engine,service=RequestProcessor");
+
+                        RequestProcessorMBeanImpl mbean = new RequestProcessorMBeanImpl();
+                        requestProcessorMBeanRegistration = bundleContext.registerService(RequestProcessorMBean.class, mbean, mbeanProps);
+                        requestProcessor.setMBean(mbean);
+                    } catch (Throwable t) {
+                        log.debug("Unable to register mbean");
+                    }
+
+                    // provide the SlingRequestProcessor service
+                    Hashtable<String, String> srpProps = new Hashtable<>();
+                    srpProps.put(Constants.SERVICE_VENDOR, "The Apache Software Foundation");
+                    srpProps.put(Constants.SERVICE_DESCRIPTION, "Sling Request Processor");
+                    requestProcessorRegistration = bundleContext.registerService(
+                            SlingRequestProcessor.class, requestProcessor, srpProps);
+                } finally {
+                    asyncActivation.countDown();
+                }
+            }
+        };
+        thread.setDaemon(true);
+        thread.start();
     }
 
-    private void registerOnInit(BundleContext bundleContext) {
-        // now that the sling main servlet is registered with the HttpService
-        // and initialized we can register the servlet context
-        slingServletContext = new SlingServletContext(bundleContext, this);
+    @Deactivate
+    protected void deactivate() {
+        if (!awaitQuietly(asyncActivation, 30)) {
+            log.warn("Async activation did not complete within 30 seconds of 'deactivate' " +
+                     "being called. There is a risk that objects are not properly destroyed.");
+        }
+        unregisterSlingServletContext();
 
-        // register render filters already registered after registration with
-        // the HttpService as filter initialization may cause the servlet
-        // context to be required (see SLING-42)
-        filterManager = new ServletFilterManager(bundleContext,
-            slingServletContext);
-        filterManager.open();
-        requestProcessor.setFilterManager(filterManager);
+        // unregister request recorder plugin
+        try {
+            RequestHistoryConsolePlugin.destroyPlugin();
+        } catch (Throwable t) {
+            log.debug(
+                    "Problem unregistering web console request recorder plugin.", t);
+        }
 
-        // initialize requestListenerManager
-        requestListenerManager = new RequestListenerManager( bundleContext, slingServletContext );
+        // third unregister and destroy the sling main servlet
+        // unregister servlet
+        if ( this.servletRegistration != null ) {
+            this.servletRegistration.unregister();
+            this.servletRegistration = null;
+        }
 
-        // Setup configuration printer
-        this.printerRegistration = WebConsoleConfigPrinter.register(bundleContext, filterManager);
-    }
+        // dispose of request listener manager after unregistering the servlet
+        // to prevent a potential NPE in the service method
+        if ( this.requestListenerManager != null ) {
+            this.requestListenerManager.dispose();
+            this.requestListenerManager = null;
+        }
 
-    @Override
-    public void init() {
-        setServerInfo();
-        log.info("{} ready to serve requests", this.getServerInfo());
-        registerOnInit(bundleContext);
+        // reset the sling main servlet reference (help GC and be nice)
+        RequestData.setSlingMainServlet(null);
+
+        this.bundleContext = null;
+
+        log.info(this.getServerInfo() + " shut down");
     }
 
-    @Deactivate
-    protected void deactivate() {
+    private void unregisterSlingServletContext() {
         // unregister the sling request processor
         if (requestProcessorRegistration != null) {
             requestProcessorRegistration.unregister();
@@ -500,14 +568,6 @@ public class SlingMainServlet extends GenericServlet {
             requestProcessorMBeanRegistration = null;
         }
 
-        // unregister request recorder plugin
-        try {
-            RequestHistoryConsolePlugin.destroyPlugin();
-        } catch (Throwable t) {
-            log.debug(
-                "Problem unregistering web console request recorder plugin.", t);
-        }
-
         // this reverses the activation setup
         if ( this.printerRegistration != null ) {
             WebConsoleConfigPrinter.unregister(this.printerRegistration);
@@ -530,27 +590,6 @@ public class SlingMainServlet extends GenericServlet {
             slingServletContext.dispose();
             slingServletContext = null;
         }
-
-        // third unregister and destroy the sling main servlet
-        // unregister servlet
-        if ( this.servletRegistration != null ) {
-            this.servletRegistration.unregister();
-            this.servletRegistration = null;
-        }
-
-        // dispose of request listener manager after unregistering the servlet
-        // to prevent a potential NPE in the service method
-        if ( this.requestListenerManager != null ) {
-            this.requestListenerManager.dispose();
-            this.requestListenerManager = null;
-        }
-
-        // reset the sling main servlet reference (help GC and be nice)
-        RequestData.setSlingMainServlet(null);
-
-        this.bundleContext = null;
-
-        log.info(this.getServerInfo() + " shut down");
     }
 
     @Reference(name = "ErrorHandler", cardinality=ReferenceCardinality.OPTIONAL, policy = ReferencePolicy.DYNAMIC, unbind = "unsetErrorHandler")
index 3ec6b9a..fd7029b 100644 (file)
@@ -28,6 +28,7 @@ import java.util.EventListener;
 import java.util.Hashtable;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
 
 import javax.servlet.Filter;
 import javax.servlet.FilterRegistration;
@@ -87,10 +88,10 @@ public class SlingServletContext implements ServletContext {
 
     /**
      * The service registration of this service as ServletContext
-     * @see #SlingServletContext(SlingMainServlet)
+     * @see #SlingServletContext(BundleContext, SlingMainServlet)
      * @see #dispose()
      */
-    private final ServiceRegistration<ServletContext> registration;
+    private AtomicReference<ServiceRegistration<ServletContext>> registration = new AtomicReference<>();
 
     /**
      * Creates an instance of this class delegating some methods to the given
@@ -105,13 +106,15 @@ public class SlingServletContext implements ServletContext {
     public SlingServletContext(final BundleContext bundleContext,
             final SlingMainServlet slingMainServlet) {
         this.slingMainServlet = slingMainServlet;
+    }
 
+    public void register(BundleContext bundleContext) {
         Dictionary<String, Object> props = new Hashtable<String, Object>();
         props.put(Constants.SERVICE_DESCRIPTION, "Apache Sling ServletContext");
         props.put(Constants.SERVICE_VENDOR, "The Apache Software Foundation");
         props.put("name", SlingMainServlet.SERVLET_CONTEXT_NAME); // property to identify this context
-        registration = bundleContext.registerService(
-            ServletContext.class, this, props);
+        registration.set(bundleContext.registerService(
+                ServletContext.class, this, props));
     }
 
     /**
@@ -120,11 +123,12 @@ public class SlingServletContext implements ServletContext {
      * This method must be called <b>before</b> the sling main servlet
      * is destroyed. Otherwise the {@link #getServletContext()} method may
      * cause a {@link NullPointerException} !
-     * @see #SlingServletContext(SlingMainServlet)
+     * @see #SlingServletContext(BundleContext, SlingMainServlet)
      */
     public void dispose() {
-        if (registration != null) {
-            registration.unregister();
+        ServiceRegistration<ServletContext> localRegistration = registration.getAndSet(null);
+        if (localRegistration != null) {
+            localRegistration.unregister();
         }
     }