Copilot commented on code in PR #7870:
URL: https://github.com/apache/incubator-seata/pull/7870#discussion_r2629435161
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -113,34 +112,35 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
// Create the HttpEntity with headers and body
HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
+ HttpMethod httpMethod;
+ try {
+ httpMethod =
HttpMethod.valueOf(request.getMethod());
+ } catch (IllegalArgumentException ex) {
+ logger.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
+
response.setStatus(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
+ return;
+ }
// Forward the request
AsyncContext asyncContext =
servletRequest.startAsync();
asyncContext.setTimeout(5000L);
- ListenableFuture<ResponseEntity<byte[]>>
responseEntityFuture = asyncRestTemplate.exchange(
- URI.create(targetUrl),
-
Objects.requireNonNull(HttpMethod.resolve(request.getMethod())),
- httpEntity,
- byte[].class);
- responseEntityFuture.addCallback(new
ListenableFutureCallback<ResponseEntity<byte[]>>() {
- @Override
- public void onFailure(Throwable ex) {
- try {
- logger.error(ex.getMessage(), ex);
-
response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
- } finally {
- asyncContext.complete();
- }
- }
-
- @Override
- public void onSuccess(ResponseEntity<byte[]>
responseEntity) {
- // Copy response headers and status code
+ Thread.startVirtualThread(() -> {
+ try {
+ CompletableFuture<ResponseEntity<byte[]>>
future = CompletableFuture.supplyAsync(
+ () ->
restTemplate.exchange(
+
URI.create(targetUrl),
+ httpMethod,
+ httpEntity,
+ byte[].class))
+ // Set a shorter time than 5000L
to prevent contention between servlet
+ // containers and virtual threads
after the request times out
+ .orTimeout(4500,
TimeUnit.MILLISECONDS);
+ ResponseEntity<byte[]> responseEntity =
future.get();
Review Comment:
Calling future.get() without a timeout can block indefinitely if the timeout
set on line 137 doesn't fire properly or if there's an edge case. Consider
using future.get(timeout, TimeUnit) with the same or slightly longer timeout to
ensure consistent timeout behavior and prevent indefinite blocking, which could
cause thread resource exhaustion.
```suggestion
ResponseEntity<byte[]> responseEntity =
future.get(4500, TimeUnit.MILLISECONDS);
```
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -113,34 +112,35 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
// Create the HttpEntity with headers and body
HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
+ HttpMethod httpMethod;
+ try {
+ httpMethod =
HttpMethod.valueOf(request.getMethod());
+ } catch (IllegalArgumentException ex) {
+ logger.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
+
response.setStatus(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
+ return;
+ }
// Forward the request
AsyncContext asyncContext =
servletRequest.startAsync();
asyncContext.setTimeout(5000L);
- ListenableFuture<ResponseEntity<byte[]>>
responseEntityFuture = asyncRestTemplate.exchange(
- URI.create(targetUrl),
-
Objects.requireNonNull(HttpMethod.resolve(request.getMethod())),
- httpEntity,
- byte[].class);
- responseEntityFuture.addCallback(new
ListenableFutureCallback<ResponseEntity<byte[]>>() {
- @Override
- public void onFailure(Throwable ex) {
- try {
- logger.error(ex.getMessage(), ex);
-
response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
- } finally {
- asyncContext.complete();
- }
- }
-
- @Override
- public void onSuccess(ResponseEntity<byte[]>
responseEntity) {
- // Copy response headers and status code
+ Thread.startVirtualThread(() -> {
+ try {
+ CompletableFuture<ResponseEntity<byte[]>>
future = CompletableFuture.supplyAsync(
+ () ->
restTemplate.exchange(
+
URI.create(targetUrl),
+ httpMethod,
+ httpEntity,
+ byte[].class))
+ // Set a shorter time than 5000L
to prevent contention between servlet
+ // containers and virtual threads
after the request times out
+ .orTimeout(4500,
TimeUnit.MILLISECONDS);
+ ResponseEntity<byte[]> responseEntity =
future.get();
Review Comment:
The nested CompletableFuture.supplyAsync inside a virtual thread is
redundant and adds unnecessary complexity. Since the code is already running in
a virtual thread (line 127), the RestTemplate.exchange call can be made
directly without wrapping it in CompletableFuture.supplyAsync. Virtual threads
are designed to handle blocking I/O operations efficiently. Simply call
restTemplate.exchange directly and use CompletableFuture.orTimeout if timeout
handling is needed.
##########
namingserver/src/main/java/org/apache/seata/namingserver/filter/ConsoleRemotingFilter.java:
##########
@@ -113,34 +112,35 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
// Create the HttpEntity with headers and body
HttpEntity<byte[]> httpEntity = new
HttpEntity<>(request.getCachedBody(), headers);
+ HttpMethod httpMethod;
+ try {
+ httpMethod =
HttpMethod.valueOf(request.getMethod());
+ } catch (IllegalArgumentException ex) {
+ logger.error("Unsupported HTTP method: {}",
request.getMethod(), ex);
+
response.setStatus(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
+ return;
+ }
Review Comment:
Error handling after setting the HTTP status should include completing the
async context immediately. Currently, if an error occurs in the try-catch block
at lines 116-122 (unsupported HTTP method), the method returns without
completing the async context, which could lead to resource leaks. The
asyncContext.complete() should be called in a finally block or the early return
should be removed to ensure the virtual thread handles completion.
##########
namingserver/src/main/resources/application.yml:
##########
@@ -18,6 +18,9 @@
server:
port: 8081
spring:
+ mvc:
+ pathmatch:
+ matching-strategy: ant_path_matcher
Review Comment:
The matching strategy configuration is set to 'ant_path_matcher', but Spring
Boot 3.x uses PathPatternParser by default, which is more performant. This
configuration reverts to the legacy Ant-style path matching. Consider migrating
path patterns to be compatible with PathPatternParser unless there's a specific
reason to use the legacy matcher (e.g., patterns like '/**/*.css' that don't
work with PathPatternParser).
##########
console/src/main/java/org/apache/seata/console/filter/JwtAuthenticationTokenFilter.java:
##########
@@ -52,16 +52,16 @@ protected void doFilterInternal(HttpServletRequest request,
HttpServletResponse
String jwt = resolveToken(request);
if (jwt != null
- && !"".equals(jwt.trim())
+ && !jwt.trim().isEmpty()
&& SecurityContextHolder.getContext().getAuthentication() ==
null) {
if (this.tokenProvider.validateToken(jwt)) {
- /**
- * get auth info
- */
+ /*
+ get auth info
+ */
Authentication authentication =
this.tokenProvider.getAuthentication(jwt);
- /**
- * save user info to securityContext
- */
+ /*
+ save user info to securityContext
+ */
Review Comment:
The comment style was changed from Javadoc style (/** ... */) to block
comment style (/* ... */). While this is a minor stylistic change, it reduces
the documentation value since these comments explain the purpose of the
authentication process. Consider keeping the Javadoc style for better IDE
support and documentation generation, especially for code that could benefit
from inline documentation.
--
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]
