Atmosphere#2073 AtmosphereInterceptor.postInspect() is not called in …

…case of a handler thrown exception
Cosium · Oct 27, 2015 · 2679929 · 2679929
1 parent 1554d75
commit 2679929
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 23 deletions.
diff --git a/modules/cpr/src/main/java/org/atmosphere/cpr/AsynchronousProcessor.java b/modules/cpr/src/main/java/org/atmosphere/cpr/AsynchronousProcessor.java
@@ -28,6 +28,7 @@
 import javax.servlet.http.HttpSession;
 import java.io.IOException;
 import java.util.Collection;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.concurrent.Callable;
 import java.util.concurrent.TimeUnit;
@@ -189,38 +190,41 @@ Action action(AtmosphereRequest req, AtmosphereResponse res) throws IOException,
         }
 
         // handler interceptor lists
-        Action a = invokeInterceptors(handlerWrapper.interceptors, resource, tracing);
+        LinkedList<AtmosphereInterceptor> invokedInterceptors = handlerWrapper.interceptors;
+        Action a = invokeInterceptors(invokedInterceptors, resource, tracing);
         if (a.type() != Action.TYPE.CONTINUE && a.type() != Action.TYPE.SKIP_ATMOSPHEREHANDLER) {
             return a;
         }
 
-        // Remap occured.
-        if (req.getAttribute(FrameworkConfig.NEW_MAPPING) != null) {
-            req.removeAttribute(FrameworkConfig.NEW_MAPPING);
-            handlerWrapper = map(req);
-            if (handlerWrapper == null) {
-                logger.debug("Remap {}", resource.uuid());
-                throw new AtmosphereMappingException("Invalid state. No AtmosphereHandler maps request for " + req.getRequestURI());
+        try {
+            // Remap occured.
+            if (req.getAttribute(FrameworkConfig.NEW_MAPPING) != null) {
+                req.removeAttribute(FrameworkConfig.NEW_MAPPING);
+                handlerWrapper = map(req);
+                if (handlerWrapper == null) {
+                    logger.debug("Remap {}", resource.uuid());
+                    throw new AtmosphereMappingException("Invalid state. No AtmosphereHandler maps request for " + req.getRequestURI());
+                }
+                resource = configureWorkflow(resource, handlerWrapper, req, res);
+                resource.setBroadcaster(handlerWrapper.broadcaster);
             }
-            resource = configureWorkflow(resource, handlerWrapper, req, res);
-            resource.setBroadcaster(handlerWrapper.broadcaster);
-        }
 
-        //Unit test mock the request and will throw NPE.
-        boolean skipAtmosphereHandler = req.getAttribute(SKIP_ATMOSPHEREHANDLER.name()) != null
-                ? (Boolean) req.getAttribute(SKIP_ATMOSPHEREHANDLER.name()) : Boolean.FALSE;
-        if (!skipAtmosphereHandler) {
-            try {
-                logger.trace("\t Last: {}", handlerWrapper.atmosphereHandler.getClass().getName());
-                handlerWrapper.atmosphereHandler.onRequest(resource);
-            } catch (IOException t) {
-                resource.onThrowable(t);
-                throw t;
+            //Unit test mock the request and will throw NPE.
+            boolean skipAtmosphereHandler = req.getAttribute(SKIP_ATMOSPHEREHANDLER.name()) != null
+                    ? (Boolean) req.getAttribute(SKIP_ATMOSPHEREHANDLER.name()) : Boolean.FALSE;
+            if (!skipAtmosphereHandler) {
+                try {
+                    logger.trace("\t Last: {}", handlerWrapper.atmosphereHandler.getClass().getName());
+                    handlerWrapper.atmosphereHandler.onRequest(resource);
+                } catch (IOException t) {
+                    resource.onThrowable(t);
+                    throw t;
+                }
             }
+        } finally{
+            postInterceptors(handlerWrapper != null? handlerWrapper.interceptors: invokedInterceptors, resource);
         }
 
-        postInterceptors(handlerWrapper.interceptors, resource);
-
         Action action = resource.action();
         if (supportSession() && allowSessionTimeoutRemoval() && action.type().equals(Action.TYPE.SUSPEND)) {
             // Do not allow times out.

diff --git a/modules/cpr/src/test/java/org/atmosphere/cpr/AtmosphereInterceptorTest.java b/modules/cpr/src/test/java/org/atmosphere/cpr/AtmosphereInterceptorTest.java
@@ -16,6 +16,7 @@
 package org.atmosphere.cpr;
 
 import org.atmosphere.interceptor.InvokationOrder;
+import org.mockito.Mockito;
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
@@ -24,10 +25,12 @@
 import javax.servlet.ServletException;
 import java.io.IOException;
 import java.util.Enumeration;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.mockito.Mockito.mock;
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
 
 public class AtmosphereInterceptorTest {
 
@@ -318,4 +321,37 @@ public String toString() {
         assertEquals(framework.getAtmosphereHandlers().get("/" + AtmosphereFramework.MAPPING_REGEX).interceptors.removeFirst().toString(), "CORS Interceptor Support");
         assertEquals(framework.getAtmosphereHandlers().get("/" + AtmosphereFramework.MAPPING_REGEX).interceptors.getFirst().toString(), "XXX");
     }
+
+    @Test
+    public void postInspectOnThrown() throws Exception{
+        AtmosphereHandler handler = mock(AtmosphereHandler.class);
+        Mockito.doThrow(new RuntimeException()).when(handler).onRequest(Mockito.any(AtmosphereResource.class));
+        framework.addAtmosphereHandler("/*", handler);
+
+        final AtomicBoolean postInspected = new AtomicBoolean(false);
+        framework.interceptor(new AtmosphereInterceptor() {
+            @Override
+            public void configure(AtmosphereConfig config) {
+            }
+
+            @Override
+            public void destroy() {
+            }
+
+            @Override
+            public Action inspect(AtmosphereResource r) {
+                return Action.CONTINUE;
+            }
+
+            @Override
+            public void postInspect(AtmosphereResource r) {
+                postInspected.set(true);
+            }
+        });
+
+        try{
+            processor.service(mock(AtmosphereRequestImpl.class), AtmosphereResponseImpl.newInstance());
+        } catch(Throwable t){}
+        assertTrue(postInspected.get());
+    }
 }