Skip to content

Commit

Permalink
fix: ensure requestEnd clears Vaadin thread locals (#20687) (CP: 24.4) (
Browse files Browse the repository at this point in the history 
#20691)

* fix: ensure requestEnd clears Vaadin thread locals (#20687)

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.

* fix imports

---------

Co-authored-by: Marco Collovati <[email protected]>
  • Loading branch information
@vaadin-bot @mcollovati
vaadin-bot and mcollovati authored Dec 13, 2024
1 parent 375e795 commit e405981
Show file tree
Hide file tree
Showing 2 changed files with 125 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 @@ -1508,22 +1508,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
109 changes: 102 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,37 +15,38 @@
*/
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;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
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.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.router.RouteConfiguration;
Expand All @@ -60,6 +61,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 @@ -127,6 +130,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 @@ -601,7 +696,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 e405981

Please sign in to comment.