Skip to content

Commit

Permalink
SLING-8187 - Deadlock in SlingMainServlet after SLING-8051
Browse files Browse the repository at this point in the history 
- 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
  • Loading branch information
@jsedding
jsedding committed Jan 7, 2019
1 parent ed72ef7 commit 40bf709
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 77 deletions.
179 changes: 109 additions & 70 deletions src/main/java/org/apache/sling/engine/impl/SlingMainServlet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {

Expand Down Expand Up @@ -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}.
Expand Down Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -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")
Expand Down
18 changes: 11 additions & 7 deletions src/main/java/org/apache/sling/engine/impl/helper/SlingServletContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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));
}

/**
Expand All @@ -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();
}
}

Expand Down

0 comments on commit 40bf709

Please sign in to comment.