Copilot commented on code in PR #7822:
URL: https://github.com/apache/incubator-seata/pull/7822#discussion_r2629925806
##########
core/src/main/java/org/apache/seata/core/rpc/netty/http/filter/HttpFilterContext.java:
##########
@@ -38,6 +41,27 @@ public HttpFilterContext(
this.paramWrapperSupplier = paramWrapperSupplier;
}
+ @SuppressWarnings("unchecked")
+ public static <T> HttpFilterContext<T> getCurrentContext() {
+ return (HttpFilterContext<T>) CURRENT_CONTEXT.get();
+ }
+
Review Comment:
The unchecked cast from HttpFilterContext<?> to HttpFilterContext<T> can
cause ClassCastException at runtime if the caller expects a specific type T
that doesn't match the actual type stored in the ThreadLocal. Consider either
removing the generic type parameter T from getCurrentContext() and having
callers cast explicitly, or documenting that callers must ensure the type
matches what was set. The current implementation silences compiler warnings but
doesn't eliminate the runtime risk.
```suggestion
public static HttpFilterContext<?> getCurrentContext() {
return CURRENT_CONTEXT.get();
}
```
##########
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);
Review Comment:
The response is being set in the filter before chain.doFilter(ctx) is
called. This is testing a filter that manually sets the response, but in the
actual HttpDispatchHandler implementation, the response is set after the filter
chain completes in the executeFinalAction or error handlers. This test doesn't
accurately reflect the real execution flow where the response is set after
business logic execution, not during filter processing.
##########
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 {
Review Comment:
The test name "testAspectCanGetHttpFilterContext" doesn't clearly convey
that it's testing ThreadLocal context management. Consider a more descriptive
name like "testHttpFilterContextAvailableInThreadLocalDuringFilterExecution" or
"testThreadLocalContextSetAndClearedDuringRequest" to better describe what
aspect of the functionality is being tested.
```suggestion
void testHttpFilterContextAvailableInThreadLocalDuringFilterExecution()
throws InterruptedException, ExecutionException,
TimeoutException {
```
##########
core/src/main/java/org/apache/seata/core/rpc/netty/http/filter/HttpFilterContext.java:
##########
@@ -38,6 +41,27 @@ public HttpFilterContext(
this.paramWrapperSupplier = paramWrapperSupplier;
}
+ @SuppressWarnings("unchecked")
+ public static <T> HttpFilterContext<T> getCurrentContext() {
+ return (HttpFilterContext<T>) CURRENT_CONTEXT.get();
+ }
+
+ public static void setCurrentContext(HttpFilterContext<?> context) {
+ CURRENT_CONTEXT.set(context);
+ }
+
+ public static void clearCurrentContext() {
+ CURRENT_CONTEXT.remove();
+ }
+
+ public void setResponse(Object response) {
+ this.response.set(response);
+ }
+
+ public Object getResponse() {
+ return this.response.get();
+ }
Review Comment:
The newly added ThreadLocal static methods and response field lack
documentation. Since these are public APIs that will be used by Spring aspects
and other consumers, they should have JavaDoc comments explaining their
purpose, thread-safety guarantees, and lifecycle (e.g., when the context is set
and cleared). This is especially important for the ThreadLocal which requires
careful lifecycle management to avoid memory leaks.
##########
core/src/main/java/org/apache/seata/core/rpc/netty/http/HttpDispatchHandler.java:
##########
@@ -75,7 +75,8 @@ protected void channelRead0(ChannelHandlerContext ctx,
HttpRequest httpRequest)
HttpInvocation httpInvocation =
ControllerManager.getHttpInvocation(path);
if (httpInvocation == null) {
- sendErrorResponse(ctx, HttpResponseStatus.NOT_FOUND, false);
+ FullHttpResponse errorResponse = addErrorResponse(context,
HttpResponseStatus.NOT_FOUND);
+ sendErrorResponse(ctx, errorResponse, false);
Review Comment:
The ThreadLocal context is not set for early error responses that occur
before the HTTP_HANDLER_THREADS.execute() call. This means that if an aspect
tries to access HttpFilterContext.getCurrentContext() for these error cases, it
will return null. The context should be set earlier in the request processing
flow, or the response should not be set in the context for these early error
cases since no filter chain will execute.
##########
core/src/main/java/org/apache/seata/core/rpc/netty/http/HttpDispatchHandler.java:
##########
@@ -95,22 +96,28 @@ 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);
+ FullHttpResponse errorResponse = addErrorResponse(context,
HttpResponseStatus.BAD_REQUEST);
+ sendErrorResponse(ctx, errorResponse, false);
Review Comment:
Similar to line 78-79, this error response is created before the
HTTP_HANDLER_THREADS.execute() call, so the ThreadLocal context is not set.
This creates inconsistency where context.setResponse() is called but
HttpFilterContext.getCurrentContext() would return null if accessed from an
aspect at this point.
##########
core/src/main/java/org/apache/seata/core/rpc/netty/http/filter/HttpFilterContext.java:
##########
@@ -21,12 +21,15 @@
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Supplier;
public class HttpFilterContext<T> extends HttpContext<T> {
private final Supplier<HttpRequestParamWrapper> paramWrapperSupplier;
private volatile HttpRequestParamWrapper paramWrapper;
private final Map<String, Object> attributes = new ConcurrentHashMap<>();
+ private static final ThreadLocal<HttpFilterContext<?>> CURRENT_CONTEXT =
new ThreadLocal<>();
+ private final AtomicReference<Object> response = new AtomicReference<>();
Review Comment:
Using AtomicReference for the response field is unnecessary overhead. Since
HttpFilterContext instances are not shared across threads (each request gets
its own instance) and the response is only set once after the handler
completes, a simple volatile field or even a regular field would be sufficient.
The AtomicReference adds unnecessary atomic operations without providing
meaningful benefits in this 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]
