From 5b0a8474c115e93a6f0aeb3ccd4b25b1ec69bbc2 Mon Sep 17 00:00:00 2001
From: Marco Collovati <marco@vaadin.com>
Date: Fri, 13 Dec 2024 10:05:55 +0100
Subject: [PATCH] fix: ensure requestEnd clears Vaadin thread locals (#20687)
 (#20692)

Makes sure that Vaadin thread locals are cleared even if something
fails durung requestEnd execution. It also wraps Vaadin interceptors
execution in a try/catch block to ensure all of them are invoked
and that potential failures does not affect the continuation of
requestEnd method.
---
 .../com/vaadin/flow/server/VaadinService.java | 26 +++++++------
 .../vaadin/flow/server/VaadinServiceTest.java | 39 ++++++++++++++++++-
 2 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java b/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
index a1e41d15fcc..3ebfc1626f8 100644
--- a/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
+++ b/flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
@@ -1475,19 +1475,23 @@ public void requestStart(VaadinRequest request, VaadinResponse response) {
      */
     public void requestEnd(VaadinRequest request, VaadinResponse response,
             VaadinSession session) {
-        if (session != null) {
-            assert VaadinSession.getCurrent() == session;
-            session.lock();
-            try {
-                cleanupSession(session);
-                final long duration = (System.nanoTime() - (Long) request
-                        .getAttribute(REQUEST_START_TIME_ATTRIBUTE)) / 1000000;
-                session.setLastRequestDuration(duration);
-            } finally {
-                session.unlock();
+        try {
+            if (session != null) {
+                assert VaadinSession.getCurrent() == session;
+                session.lock();
+                try {
+                    cleanupSession(session);
+                    final long duration = (System.nanoTime() - (Long) request
+                            .getAttribute(REQUEST_START_TIME_ATTRIBUTE))
+                            / 1000000;
+                    session.setLastRequestDuration(duration);
+                } finally {
+                    session.unlock();
+                }
             }
+        } finally {
+            CurrentInstance.clearAll();
         }
-        CurrentInstance.clearAll();
     }
 
     /**
diff --git a/flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java b/flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java
index 1c8097fca0d..f72e07992e3 100644
--- a/flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java
+++ b/flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java
@@ -12,7 +12,6 @@
 import javax.servlet.ServletConfig;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpSessionBindingEvent;
-
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -87,6 +86,44 @@ protected List<RequestHandler> createRequestHandlers()
         }
     }
 
+    @Test
+    public void requestEnd_serviceFailure_threadLocalsCleared() {
+        MockVaadinServletService service = new MockVaadinServletService() {
+            @Override
+            void cleanupSession(VaadinSession session) {
+                throw new RuntimeException("BOOM");
+            }
+        };
+        service.init();
+
+        VaadinRequest request = Mockito.mock(VaadinRequest.class);
+        VaadinResponse response = Mockito.mock(VaadinResponse.class);
+        service.requestStart(request, response);
+
+        Assert.assertSame(service, VaadinService.getCurrent());
+        Assert.assertSame(request, VaadinRequest.getCurrent());
+        Assert.assertSame(response, VaadinResponse.getCurrent());
+
+        VaadinSession session = Mockito.mock(VaadinSession.class);
+        VaadinSession.setCurrent(session);
+
+        try {
+            service.requestEnd(request, response, session);
+            Assert.fail("Should have thrown an exception");
+        } catch (Exception e) {
+            Assert.assertNull("VaadinService.current",
+                    VaadinService.getCurrent());
+            Assert.assertNull("VaadinSession.current",
+                    VaadinSession.getCurrent());
+            Assert.assertNull("VaadinRequest.current",
+                    VaadinRequest.getCurrent());
+            Assert.assertNull("VaadinResponse.current",
+                    VaadinResponse.getCurrent());
+        } finally {
+            CurrentInstance.clearAll();
+        }
+    }
+
     @Test
     public void should_reported_routing_server() {