-
Notifications
You must be signed in to change notification settings - Fork 967
Description
When a ServiceRequestContext is pushed, Armeria validates it against any existing ClientRequestContext stored in the thread-local.
armeria/core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java
Line 241 in 95ed9e1
| if (RequestContextUtil.equalsIgnoreWrapper(oldCtx.root(), this)) { |
If
ClientRequestContext.root() is null, the validation logic throws an IllegalStateException since it thinks the context is leaked.
Normally, ClientRequestContext.root() is set when a client is executed within a service, so this check works as expected.
armeria/core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java
Lines 256 to 260 in 35126bd
| this(null, meterRegistry, sessionProtocol, | |
| id, method, reqTarget, endpointGroup, options, req, rpcReq, requestOptions, | |
| serviceRequestContext(), /* responseCancellationScheduler */ null, | |
| requestStartTimeNanos, requestStartTimeMicros); | |
| } |
However, if a client request is executed outside a ServiceRequestContext, which can easily happen during asynchronous composition, then ClientRequestContext.root() remains null.
clientA.execute()
.thenComposeAsync(unused -> clientB.execute());In such cases, even though the request was initiated and completed from a valid ServiceRequestContext, Armeria incorrectly reports a request context leak.
This issue is especially reproducible with RetryingClient. Since its responses are completed on a context-aware event loop, a ClientRequestContext is always present in the thread-local.
armeria/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java
Line 246 in 1051e96
| final HttpResponse res = HttpResponse.of(responseFuture, ctx.eventLoop()); |
That is triggering the problematic validation. In contrast, without
RetryingClient, responses complete on the channel’s event loop and the warning does not occur.armeria/core/src/main/java/com/linecorp/armeria/client/Http2ResponseDecoder.java
Lines 286 to 288 in 603fc86
| if (endOfStream) { | |
| res.close(); | |
| } |
I do think that Armeria should not report a RequestContext leak when the client request actually starts and completes within a valid service scope.