jsedding closed pull request #6: SLING-8187 - Deadlock in SlingMainServlet
after SLING-8051
URL: https://github.com/apache/sling-org-apache-sling-engine/pull/6
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
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.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 @@
* <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 @@
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 void service(ServletRequest req, ServletResponse
res)
// ---------- 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 @@ protected void activate(final BundleContext
bundleContext,
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 @@ protected void deactivate() {
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 @@ protected void deactivate() {
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.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 @@
/**
* 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 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 SlingServletContext(final BundleContext
bundleContext,
* 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();
}
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
