Skip to content

Commit

Permalink
fix: ensure requestEnd clears Vaadin thread locals (#20687) (#20693)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mcollovati authored Dec 13, 2024
1 parent 919aad7 commit 5b87120
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 11 deletions.
26 changes: 15 additions & 11 deletions flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
Original file line number Diff line number Diff line change
Expand Up @@ -1466,19 +1466,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();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,44 @@ private String createCriticalNotification(String caption, String message,
details, url);
}

@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 testFireSessionDestroy()
throws ServletException, ServiceException {
Expand Down

0 comments on commit 5b87120

Please sign in to comment.