You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by js...@apache.org on 2019/01/07 12:36:21 UTC
[sling-org-apache-sling-engine] branch master updated: SLING-8187 -
Deadlock in SlingMainServlet after SLING-8051
This is an automated email from the ASF dual-hosted git repository.
jsedding pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-engine.git
The following commit(s) were added to refs/heads/master by this push:
new 40bf709 SLING-8187 - Deadlock in SlingMainServlet after SLING-8051
40bf709 is described below
commit 40bf709c2e32836a2406137c1e94e67c7061cc04
Author: Julian Sedding <js...@apache.org>
AuthorDate: Thu Dec 20 14:05:49 2018 +0100
SLING-8187 - Deadlock in SlingMainServlet after SLING-8051
- 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
---
.../apache/sling/engine/impl/SlingMainServlet.java | 179 +++++++++++++--------
.../engine/impl/helper/SlingServletContext.java | 18 ++-
2 files changed, 120 insertions(+), 77 deletions(-)
diff --git a/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java b/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java
index 458441e..e9cd272 100644
--- a/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java
+++ b/src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java
@@ -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")
diff --git a/src/main/java/org/apache/sling/engine/impl/helper/SlingServletContext.java b/src/main/java/org/apache/sling/engine/impl/helper/SlingServletContext.java
index 3ec6b9a..fd7029b 100644
--- a/src/main/java/org/apache/sling/engine/impl/helper/SlingServletContext.java
+++ b/src/main/java/org/apache/sling/engine/impl/helper/SlingServletContext.java
@@ -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();
}
}