WW-5414 Always call afterInvocation even in case of exception

apache · May 11, 2024 · 4dfbe09 · 4dfbe09
1 parent 649760d
commit 4dfbe09
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 10 deletions.
diff --git a/core/pom.xml b/core/pom.xml
@@ -260,6 +260,12 @@
             <scope>test</scope>
         </dependency>
 
+        <dependency>
+            <groupId>org.awaitility</groupId>
+            <artifactId>awaitility</artifactId>
+            <scope>test</scope>
+        </dependency>
+
         <dependency>
             <groupId>junit</groupId>
             <artifactId>junit</artifactId>

diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java b/core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java
@@ -20,6 +20,8 @@
 
 import com.opensymphony.xwork2.ActionContext;
 import com.opensymphony.xwork2.ActionInvocation;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 
 import java.io.Serializable;
 
@@ -30,6 +32,8 @@ public class StrutsBackgroundProcess implements BackgroundProcess, Serializable
 
     private static final long serialVersionUID = 3884464776311686443L;
 
+    private static final Logger LOG = LogManager.getLogger(StrutsBackgroundProcess.class);
+
     private final String threadName;
     private final int threadPriority;
 
@@ -44,8 +48,8 @@ public class StrutsBackgroundProcess implements BackgroundProcess, Serializable
     /**
      * Constructs a background process
      *
-     * @param invocation The action invocation
-     * @param threadName The name of background thread
+     * @param invocation     The action invocation
+     * @param threadName     The name of background thread
      * @param threadPriority The priority of background thread
      */
     public StrutsBackgroundProcess(ActionInvocation invocation, String threadName, int threadPriority) {
@@ -61,11 +65,19 @@ public BackgroundProcess prepare() {
                 try {
                     beforeInvocation();
                     result = invocation.invokeActionOnly();
-                    afterInvocation();
                 } catch (Exception e) {
+                    LOG.warn("Exception during invokeActionOnly() execution", e);
                     exception = e;
                 } finally {
-                  done = true;
+                    try {
+                        afterInvocation();
+                    } catch (Exception ex) {
+                        if (exception == null) {
+                            exception = ex;
+                        }
+                        LOG.warn("Exception during afterInvocation() execution", ex);
+                    }
+                    done = true;
                 }
             });
             processThread.setName(threadName);

diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java
@@ -26,6 +26,7 @@
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
+import java.io.NotSerializableException;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
 import java.security.SecureRandom;
@@ -41,6 +42,8 @@
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicInteger;
 
+import static org.awaitility.Awaitility.await;
+
 /**
  * Test case for BackgroundProcessTest.
  */
@@ -59,9 +62,9 @@ public void testSerializeDeserialize() throws Exception {
         invocation.setInvocationContext(ActionContext.getContext());
 
         StrutsBackgroundProcess bp = (StrutsBackgroundProcess) new StrutsBackgroundProcess(
-            invocation,
-            "BackgroundProcessTest.testSerializeDeserialize",
-            Thread.MIN_PRIORITY
+                invocation,
+                "BackgroundProcessTest.testSerializeDeserialize",
+                Thread.MIN_PRIORITY
         ).prepare();
         executor.execute(bp);
 
@@ -120,6 +123,31 @@ public void testMultipleProcesses() throws InterruptedException {
         assertEquals(100, mutableState.get());
     }
 
+    public void testErrorableProcesses1() {
+        MockActionInvocationWithActionInvoker invocation = new MockActionInvocationWithActionInvoker(() -> {
+            throw new IllegalStateException("boom");
+        });
+
+        BackgroundProcess bp = new ErrorableBackgroundProcess(invocation, null).prepare();
+        executor.execute(bp);
+
+        await().atLeast(100, TimeUnit.MILLISECONDS).until(bp::isDone);
+
+        assertTrue("afterInvocation not called in case of exception", ((ErrorableBackgroundProcess) bp).isDoneAfter());
+    }
+
+    public void testErrorableProcesses2() {
+        MockActionInvocationWithActionInvoker invocation = new MockActionInvocationWithActionInvoker(() -> "done");
+
+        IllegalStateException expected = new IllegalStateException("after!");
+        BackgroundProcess bp = new ErrorableBackgroundProcess(invocation, expected).prepare();
+        executor.execute(bp);
+
+        await().atLeast(100, TimeUnit.MILLISECONDS).until(bp::isDone);
+
+        assertEquals(expected, bp.getException());
+    }
+
     public void testUnpreparedProcess() throws ExecutionException, InterruptedException, TimeoutException {
         // given
         MockActionInvocationWithActionInvoker invocation = new MockActionInvocationWithActionInvoker(() -> "done");
@@ -147,7 +175,8 @@ public String invokeActionOnly() throws Exception {
     }
 
     private static class NotSerializableException extends Exception {
-        private MockHttpServletRequest notSerializableField;
+        @SuppressWarnings("unused")
+        private final MockHttpServletRequest notSerializableField;
 
         NotSerializableException(MockHttpServletRequest notSerializableField) {
             this.notSerializableField = notSerializableField;
@@ -170,10 +199,29 @@ public void run() {
             super.run();
         }
     }
+}
+
+class ErrorableBackgroundProcess extends StrutsBackgroundProcess {
+
+    private final Exception afterException;
+    private boolean doneAfter;
+
+    public ErrorableBackgroundProcess(ActionInvocation invocation, Exception afterException) {
+        super(invocation, "errorabale process", Thread.NORM_PRIORITY);
+        this.afterException = afterException;
+    }
 
     @Override
     protected void afterInvocation() throws Exception {
-        super.afterInvocation();
-        lock.notify();
+        if (afterException != null) {
+            throw afterException;
+        } else {
+            super.afterInvocation();
+            doneAfter = true;
+        }
+    }
+
+    public boolean isDoneAfter() {
+        return doneAfter;
     }
 }
diff --git a/pom.xml b/pom.xml
@@ -774,6 +774,13 @@
                 <scope>test</scope>
             </dependency>
 
+            <dependency>
+                <groupId>org.awaitility</groupId>
+                <artifactId>awaitility</artifactId>
+                <version>4.2.1</version>
+                <scope>test</scope>
+            </dependency>
+
             <dependency>
                 <groupId>javax.servlet</groupId>
                 <artifactId>javax.servlet-api</artifactId>