Copilot commented on code in PR #7822:
URL: https://github.com/apache/incubator-seata/pull/7822#discussion_r2642594090
##########
core/src/main/java/org/apache/seata/core/rpc/netty/http/HttpDispatchHandler.java:
##########
@@ -72,47 +72,60 @@ protected void channelRead0(ChannelHandlerContext ctx,
HttpRequest httpRequest)
() -> new HttpRequestParamWrapper(
queryParams, bodyParseResult.getFormParams(),
headerParams, bodyParseResult.getJsonParams()));
- HttpInvocation httpInvocation =
ControllerManager.getHttpInvocation(path);
-
- if (httpInvocation == null) {
- sendErrorResponse(ctx, HttpResponseStatus.NOT_FOUND, false);
- return;
- }
+ HttpFilterContext.setCurrentContext(context);
+ try {
+ HttpInvocation httpInvocation =
ControllerManager.getHttpInvocation(path);
- context.setAttribute("httpInvocation", httpInvocation);
- context.setAttribute("httpController", httpInvocation.getController());
- context.setAttribute("handleMethod", httpInvocation.getMethod());
+ if (httpInvocation == null) {
+ FullHttpResponse errorResponse = addErrorResponse(context,
HttpResponseStatus.NOT_FOUND);
+ sendErrorResponse(ctx, errorResponse, false);
+ return;
+ }
- ObjectNode requestDataNode = OBJECT_MAPPER.createObjectNode();
- requestDataNode.set("param",
ParameterParser.convertParamMap(queryParams));
- if (httpRequest.method() == HttpMethod.POST &&
bodyParseResult.getBodyNode() != null) {
- requestDataNode.set("body", bodyParseResult.getBodyNode());
- }
+ context.setAttribute("httpInvocation", httpInvocation);
+ context.setAttribute("httpController",
httpInvocation.getController());
+ context.setAttribute("handleMethod", httpInvocation.getMethod());
- Object[] args;
- try {
- args = ParameterParser.getArgValues(
- httpInvocation.getParamMetaData(),
httpInvocation.getMethod(), requestDataNode, context);
- } catch (Exception e) {
- LOGGER.error("Error parsing request arguments: {}",
e.getMessage(), e);
- sendErrorResponse(ctx, HttpResponseStatus.BAD_REQUEST, false);
- return;
- }
- context.setAttribute("args", args);
+ ObjectNode requestDataNode = OBJECT_MAPPER.createObjectNode();
+ requestDataNode.set("param",
ParameterParser.convertParamMap(queryParams));
+ if (httpRequest.method() == HttpMethod.POST &&
bodyParseResult.getBodyNode() != null) {
+ requestDataNode.set("body", bodyParseResult.getBodyNode());
+ }
- // Execute filter chain in HTTP thread pool
- HttpRequestFilterChain filterChain =
HttpRequestFilterManager.getFilterChain(this::executeFinalAction);
- HTTP_HANDLER_THREADS.execute(() -> {
+ Object[] args;
try {
- filterChain.doFilter(context);
- } catch (HttpRequestFilterException e) {
- LOGGER.warn("Request blocked by filter: {}", e.getMessage());
- sendErrorResponse(ctx, HttpResponseStatus.BAD_REQUEST, false);
+ args = ParameterParser.getArgValues(
+ httpInvocation.getParamMetaData(),
httpInvocation.getMethod(), requestDataNode, context);
} catch (Exception e) {
- LOGGER.error("Unexpected error during request processing: {}",
e.getMessage(), e);
- sendErrorResponse(ctx,
HttpResponseStatus.INTERNAL_SERVER_ERROR, false);
+ LOGGER.error("Error parsing request arguments: {}",
e.getMessage(), e);
+ FullHttpResponse errorResponse = addErrorResponse(context,
HttpResponseStatus.BAD_REQUEST);
+ sendErrorResponse(ctx, errorResponse, false);
+ return;
}
- });
+ context.setAttribute("args", args);
+
+ // Execute filter chain in HTTP thread pool
+ HttpRequestFilterChain filterChain =
HttpRequestFilterManager.getFilterChain(this::executeFinalAction);
+ HTTP_HANDLER_THREADS.execute(() -> {
+ HttpFilterContext.setCurrentContext(context);
+ try {
+ filterChain.doFilter(context);
+ } catch (HttpRequestFilterException e) {
+ LOGGER.warn("Request blocked by filter: {}",
e.getMessage());
+ FullHttpResponse errorResponse = addErrorResponse(context,
HttpResponseStatus.BAD_REQUEST);
+ sendErrorResponse(ctx, errorResponse, false);
+ } catch (Exception e) {
+ LOGGER.error("Unexpected error during request processing:
{}", e.getMessage(), e);
+ FullHttpResponse errorResponse =
+ addErrorResponse(context,
HttpResponseStatus.INTERNAL_SERVER_ERROR);
+ sendErrorResponse(ctx, errorResponse, false);
+ } finally {
+ HttpFilterContext.clearCurrentContext();
+ }
+ });
+ } finally {
+ HttpFilterContext.clearCurrentContext();
+ }
Review Comment:
The ThreadLocal context management has a threading issue. The context is set
on line 75 (Netty I/O thread) and then cleared on line 127 (also Netty I/O
thread), but the actual filter processing happens in a different thread
(HTTP_HANDLER_THREADS pool, lines 109-125).
Since the processing happens asynchronously in HTTP_HANDLER_THREADS, the
context set on line 75 serves no purpose - it's immediately cleared on line 127
before the HTTP_HANDLER_THREADS task even starts. Only the context set on line
110 within the HTTP_HANDLER_THREADS task is meaningful.
Consider removing the setCurrentContext call on line 75 and the outer
try-finally block (lines 76 and 126-128), since they operate on a different
thread's ThreadLocal than where the actual processing occurs.
##########
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")
Review Comment:
The @SuppressWarnings("unchecked") annotation on getCurrentContext() is
unnecessary. The method returns HttpFilterContext<?> directly from the
ThreadLocal without any casting, so there are no unchecked operations to
suppress.
```suggestion
```
--
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]
