Skip to content

Commit

Permalink
fix: ensure requestEnd clears Vaadin thread locals (#20687)
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
mcollovati authored Dec 13, 2024
1 parent 0e059c2 commit c06a0a3
Show file tree
Hide file tree
Showing 2 changed files with 123 additions and 20 deletions.
36 changes: 23 additions & 13 deletions flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1565,22 +1565,32 @@ public void requestStart(VaadinRequest request, VaadinResponse response) {
*/
public void requestEnd(VaadinRequest request, VaadinResponse response,
VaadinSession session) {
vaadinRequestInterceptors
.forEach(requestInterceptor -> requestInterceptor
.requestEnd(request, response, session));
if (session != null) {
assert VaadinSession.getCurrent() == session;
session.lock();
vaadinRequestInterceptors.forEach(requestInterceptor -> {
try {
cleanupSession(session);
final long duration = (System.nanoTime() - (Long) request
.getAttribute(REQUEST_START_TIME_ATTRIBUTE)) / 1000000;
session.setLastRequestDuration(duration);
} finally {
session.unlock();
requestInterceptor.requestEnd(request, response, session);
} catch (Exception ex) {
getLogger().error(
"Error occurred while processing Vaadin request interceptor {}",
requestInterceptor.getClass().getName(), ex);
}
});
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
107 changes: 100 additions & 7 deletions flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,9 @@
*/
package com.vaadin.flow.server;

import com.vaadin.flow.component.DetachEvent;
import com.vaadin.flow.component.UI;
import com.vaadin.flow.i18n.DefaultI18NProvider;
import com.vaadin.flow.i18n.I18NProvider;
import net.jcip.annotations.NotThreadSafe;
import jakarta.servlet.ServletConfig;
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpSessionBindingEvent;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
Expand All @@ -38,17 +32,22 @@
import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Collectors;

import net.jcip.annotations.NotThreadSafe;
import org.junit.Assert;
import org.junit.Test;
import org.mockito.MockedStatic;
import org.mockito.Mockito;

import com.vaadin.flow.component.Component;
import com.vaadin.flow.component.DetachEvent;
import com.vaadin.flow.component.Tag;
import com.vaadin.flow.component.UI;
import com.vaadin.flow.di.Instantiator;
import com.vaadin.flow.di.InstantiatorFactory;
import com.vaadin.flow.di.Lookup;
import com.vaadin.flow.function.DeploymentConfiguration;
import com.vaadin.flow.i18n.DefaultI18NProvider;
import com.vaadin.flow.i18n.I18NProvider;
import com.vaadin.flow.internal.CurrentInstance;
import com.vaadin.flow.internal.UsageStatistics;
import com.vaadin.flow.internal.menu.MenuRegistry;
Expand All @@ -69,6 +68,8 @@
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;

/**
*
Expand Down Expand Up @@ -148,6 +149,98 @@ 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 requestEnd_interceptorFailure_allInterceptorsInvoked_doNotThrowAndThreadLocalsCleared() {
VaadinRequestInterceptor interceptor1 = Mockito
.mock(VaadinRequestInterceptor.class);
VaadinRequestInterceptor interceptor2 = Mockito
.mock(VaadinRequestInterceptor.class);
Mockito.doThrow(new RuntimeException("BOOM")).when(interceptor2)
.requestEnd(any(), any(), any());
VaadinRequestInterceptor interceptor3 = Mockito
.mock(VaadinRequestInterceptor.class);

VaadinServiceInitListener initListener = event -> {
event.addVaadinRequestInterceptor(interceptor1);
event.addVaadinRequestInterceptor(interceptor2);
event.addVaadinRequestInterceptor(interceptor3);
};
MockInstantiator instantiator = new MockInstantiator(initListener);
MockVaadinServletService service = new MockVaadinServletService();
service.init(instantiator);

Map<String, Object> attributes = new HashMap<>();
VaadinRequest request = Mockito.mock(VaadinRequest.class);
Mockito.when(request.getAttribute(anyString()))
.then(i -> attributes.get(i.getArgument(0, String.class)));
Mockito.doAnswer(i -> attributes.put(i.getArgument(0, String.class),
i.getArgument(1))).when(request)
.setAttribute(anyString(), any());
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.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() {

Expand Down Expand Up @@ -735,7 +828,7 @@ private InstantiatorFactory createInstantiatorFactory(Lookup lookup) {
InstantiatorFactory factory = Mockito.mock(InstantiatorFactory.class);

Instantiator instantiator = Mockito.mock(Instantiator.class);
Mockito.when((factory.createInstantitor(Mockito.any())))
Mockito.when((factory.createInstantitor(any())))
.thenReturn(instantiator);

return factory;
Expand Down

0 comments on commit c06a0a3

Please # to comment.