Copilot commented on code in PR #7822:
URL: https://github.com/apache/incubator-seata/pull/7822#discussion_r2626287062
##########
core/src/test/java/org/apache/seata/core/rpc/netty/http/HttpDispatchHandlerTest.java:
##########
@@ -304,6 +351,84 @@ void testRequestWithUnexpectedExceptionDuringFilter()
throws Exception {
}
}
+ @Test
+ void testAspectCanGetHttpFilterContext() throws InterruptedException,
ExecutionException, TimeoutException {
+ class MockHttpAspect {
+ public void beforeFilter() {
+ HttpFilterContext<?> context =
HttpFilterContext.getCurrentContext();
+ assertNotNull(context, "get request and response");
+
+ HttpRequest request = (HttpRequest) context.getRequest();
+ assertNotNull(request);
+ assertEquals("/test", request.uri());
+ assertEquals(HttpMethod.GET, request.method());
+ assertEquals("test-invocation",
context.getAttribute("testKey"));
+ }
+
+ public void afterFilter() {
+ HttpFilterContext<?> context =
HttpFilterContext.getCurrentContext();
+ assertNotNull(context, "get request and response");
+
+ FullHttpResponse response = (FullHttpResponse)
context.getResponse();
+ assertNotNull(response);
+ assertEquals(HttpResponseStatus.OK, response.status());
+ }
+
+ public void afterFinally() {
+ assertNull(HttpFilterContext.getCurrentContext(), "clean the
request and response");
+ }
+ }
+
+ MockHttpAspect mockAspect = new MockHttpAspect();
+
+ HttpRequestFilter businessFilter = new HttpRequestFilter() {
+ @Override
+ public void doFilter(HttpFilterContext<?> ctx,
HttpRequestFilterChain chain)
+ throws HttpRequestFilterException {
+ mockAspect.beforeFilter();
+
+ FullHttpResponse response =
+ new DefaultFullHttpResponse(HttpVersion.HTTP_1_1,
HttpResponseStatus.OK, Unpooled.EMPTY_BUFFER);
+ ctx.setResponse(response);
+
+ chain.doFilter(ctx);
+
+ mockAspect.afterFilter();
+ }
+
+ @Override
+ public boolean shouldApply() {
+ return false;
+ }
+ };
+
+ HttpRequestFilterChain filterChain = new
HttpRequestFilterChain(Arrays.asList(businessFilter), ctx -> {});
+
+ HttpFilterContext<HttpRequest> context = new HttpFilterContext<>(
+ testHttpRequest,
+ mockCtx,
+ true,
+ HttpContext.HTTP_1_1,
+ () -> new HttpRequestParamWrapper(null, null, null, null));
+ context.setAttribute("testKey", "test-invocation");
+
+ testExecutor
+ .submit(() -> {
+ try {
+ HttpFilterContext.setCurrentContext(context);
+ filterChain.doFilter(context);
+ } catch (HttpRequestFilterException e) {
+ fail("Filter failure: " + e.getMessage());
+ } finally {
+ HttpFilterContext.clearCurrentContext();
+ mockAspect.afterFinally();
+ }
+ })
+ .get(1, TimeUnit.SECONDS);
+
+ assertNull(HttpFilterContext.getCurrentContext());
+ }
Review Comment:
The test testAspectCanGetHttpFilterContext validates the ThreadLocal pattern
in isolation, but doesn't actually test whether HttpDispatchHandler correctly
sets the context in the executor thread. The test manually calls
setCurrentContext inside its own executor, which is the correct pattern, but
the actual HttpDispatchHandler implementation (line 105 in
HttpDispatchHandler.java) sets the context on the wrong thread.
Consider adding an integration test that goes through the full
HttpDispatchHandler flow and verifies that getCurrentContext returns a non-null
value when called from within a filter's doFilter method.
##########
core/src/main/java/org/apache/seata/core/rpc/netty/http/HttpDispatchHandler.java:
##########
@@ -95,22 +95,25 @@ protected void channelRead0(ChannelHandlerContext ctx,
HttpRequest httpRequest)
httpInvocation.getParamMetaData(),
httpInvocation.getMethod(), requestDataNode, context);
} catch (Exception e) {
LOGGER.error("Error parsing request arguments: {}",
e.getMessage(), e);
- sendErrorResponse(ctx, HttpResponseStatus.BAD_REQUEST, false);
+ sendErrorResponse(ctx, HttpResponseStatus.BAD_REQUEST, false,
context);
return;
}
context.setAttribute("args", args);
// Execute filter chain in HTTP thread pool
HttpRequestFilterChain filterChain =
HttpRequestFilterManager.getFilterChain(this::executeFinalAction);
+ HttpFilterContext.setCurrentContext(context);
HTTP_HANDLER_THREADS.execute(() -> {
Review Comment:
ThreadLocal context is set on the wrong thread. The setCurrentContext call
is executed on the Netty event loop thread (channelRead0), but the context is
only used and cleared in the HTTP_HANDLER_THREADS executor. This means:
1. The context will never be accessible in the executor thread where filters
and business logic run
2. The context will remain set (and never cleared) in the Netty event loop
thread, causing a memory leak
The setCurrentContext call should be moved inside the executor's Runnable,
before the filterChain.doFilter call.
```suggestion
HTTP_HANDLER_THREADS.execute(() -> {
HttpFilterContext.setCurrentContext(context);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
