Provide a way to complete a ClientRequestContext in Preprocessor - #6466
Conversation
minwoox
left a comment
minwoox
left a comment
There was a problem hiding this comment.
Looks great. 👍
Left small suggestions. 😉
| if (!initializationTriggeredUpdater.compareAndSet(this, 0, 1)) { | ||
| return false; | ||
| } | ||
| acquireEventLoop(endpointGroup); |
There was a problem hiding this comment.
Probably, we don't need to call initializeResponseCancellationScheduler in acquireEventLoop?
| @@ -179,6 +179,9 @@ O executeWithFallback(U execution, | |||
| try { | |||
| return execution.execute(ctx, req); | |||
There was a problem hiding this comment.
What do you think about moving the call to completeLogIfIncomplete from line 111 to here? It seems like it would prevent the case where an http response is returned in the pre decorator.
There was a problem hiding this comment.
Shouldn't we also change this line?
futureConverter, errorResponseFactory, req, false);
Additionally, this isn't related to this PR but shouldn't tryCompleteLog be true in these lines? cc @ikhoon
There was a problem hiding this comment.
Shouldn't we also change this line? futureConverter, errorResponseFactory, req, false);
Understood that the intention is that completeLogIfIncomplete is already registered when preclients are invoked, and hence another callback doesn't need to be added
There was a problem hiding this comment.
Additionally, this isn't related to this PR but shouldn't tryCompleteLog be true in these lines? cc @ikhoon
tryCompleteLog should be false since RetryingClient registers its own callbacks which are optimized for RetryRule.
There was a problem hiding this comment.
Ah I missed it. Thanks for the explanation. 😉
@jrhee17 Would you mind adding a comment for that?
e.g. // tryCompleteLog is false because we handle it in completeLogIfBytesNotTransferred.
There was a problem hiding this comment.
Side note) I couldn’t immediately tell the difference between the two just from the function name executeWithFallback. Renaming them to something more explicit like executePreClient and executeClient would make the context clearer during code review.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6466 +/- ##
============================================
- Coverage 74.46% 74.18% -0.28%
- Complexity 22234 23286 +1052
============================================
Files 1963 2089 +126
Lines 82437 87013 +4576
Branches 10764 11438 +674
============================================
+ Hits 61385 64549 +3164
- Misses 15918 17007 +1089
- Partials 5134 5457 +323 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughReplaces endpoint-null initialization checks with an initialization-triggered flag, adds initAndFail/initializationTriggered APIs, renames executeWithFallback → executePreClientWithFallback with unified error handling, exposes CancellationScheduler.hasEventLoop(), cancels contexts on several preprocessor/XDS error paths, and adds tests for preprocessor error behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Retry as RetryingClient
participant CtxExt as ClientRequestContextExtension
participant Util as ClientUtil
participant Ctx as DefaultClientRequestContext
Client->>Retry: execute(request)
Retry->>CtxExt: initializationTriggered()?
alt not triggered
CtxExt-->>Retry: false
Retry->>Util: initContextAndExecuteWithFallback(...)
Util->>Ctx: init() / initNow() when endpoint present
Ctx-->>Util: whenInitialized / result
Util-->>Client: response
else triggered
CtxExt-->>Retry: true
Retry->>Util: executePreClientWithFallback(...)
Util-->>Client: response
end
sequenceDiagram
participant Client
participant Util as ClientUtil
participant Pre as Preprocessor
participant CtxExt as ClientRequestContextExtension
participant Err as ErrorFactory
Client->>Util: executePreClientWithFallback(ctx, req, ...)
Util->>Pre: execute(req)
alt success
Pre-->>Util: response
Util->>Util: completeLogIfIncomplete(ctx, res)
Util-->>Client: response
else exception
Pre-->>Util: throws e
Util->>CtxExt: initializationTriggered()?
alt not triggered
CtxExt-->>Util: false
Util->>CtxExt: initAndFail(e)
end
Util->>Err: build error response from e
Util->>Util: completeLogIfIncomplete(ctx, errorRes)
Util-->>Client: errorRes
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilter.java (1)
113-114: Missing context cancellation in async error path.The async error path in the catch block at lines 113-114 does not call
ctx.cancel(e)before rethrowing, which is inconsistent with:
- The four synchronous error paths in this same file (lines 56-61, 65-69, 73-77, 96-100)
- The similar async error handling in
XdsPreprocessor.java(line 72)This omission could prevent proper context lifecycle completion when errors occur in the async endpoint selection path.
Apply this diff to add the missing cancellation:
.thenApply(endpoint0 -> { try { return execute0(delegate, ctx, req, endpoint0); } catch (Exception e) { + ctx.cancel(e); return Exceptions.throwUnsafely(e); } });
🧹 Nitpick comments (3)
core/src/main/java/com/linecorp/armeria/internal/common/CancellationScheduler.java (1)
136-136: Consider adding Javadoc for clarity.The new
hasEventLoop()method extends the public API surface of this interface. While the intent seems clear, adding a brief Javadoc comment would improve documentation consistency with other methods in the interface and clarify its purpose for users of this internal API.Example:
+ /** + * Returns {@code true} if an event loop has been assigned to this scheduler. + */ boolean hasEventLoop();core/src/test/java/com/linecorp/armeria/client/HttpPreprocessorTest.java (1)
117-135: LGTM! Comprehensive test for preprocessor failure handling.This test effectively validates that when a preprocessor throws an exception:
- The exception propagates to the caller
- Context initialization completes (
whenInitialized()is done)- Request log completes
- Event loop remains assigned
The use of Awaitility for async assertions is appropriate.
Optional refinement: The cast at line 130 to
DefaultClientRequestContextcould be more specific. If you only need theClientRequestContextExtensioninterface methods, consider casting to that interface instead:- final DefaultClientRequestContext ctx = (DefaultClientRequestContext) captor.get(); + final ClientRequestContext ctx = captor.get(); + final ClientRequestContextExtension ctxExt = ctx.as(ClientRequestContextExtension.class);core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java (1)
319-329: LGTM on the initialization check change.The switch from
endpoint() == nullto!ctxExtension.initializationTriggered()correctly aligns with the new initialization tracking semantics.Based on past review comments, consider adding a brief comment explaining why
tryCompleteLogisfalsein both branches (e.g.,// tryCompleteLog is false because we handle it in completeLogIfBytesNotTransferred).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java(1 hunks)core/src/main/java/com/linecorp/armeria/client/retry/RetryingRpcClient.java(1 hunks)core/src/main/java/com/linecorp/armeria/internal/client/ClientRequestContextExtension.java(1 hunks)core/src/main/java/com/linecorp/armeria/internal/client/ClientUtil.java(1 hunks)core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java(15 hunks)core/src/main/java/com/linecorp/armeria/internal/client/TailPreClient.java(1 hunks)core/src/main/java/com/linecorp/armeria/internal/common/CancellationScheduler.java(1 hunks)core/src/main/java/com/linecorp/armeria/internal/common/DefaultCancellationScheduler.java(1 hunks)core/src/main/java/com/linecorp/armeria/internal/common/NoopCancellationScheduler.java(1 hunks)core/src/test/java/com/linecorp/armeria/client/HttpPreprocessorTest.java(3 hunks)grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcPreprocessorTest.java(1 hunks)it/xds-client/src/test/java/com/linecorp/armeria/xds/it/PreprocessorErrorTest.java(1 hunks)xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilter.java(2 hunks)xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsPreprocessor.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internal.- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilter.javagrpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcPreprocessorTest.javacore/src/main/java/com/linecorp/armeria/client/retry/RetryingRpcClient.javacore/src/main/java/com/linecorp/armeria/internal/client/ClientRequestContextExtension.javacore/src/main/java/com/linecorp/armeria/internal/common/CancellationScheduler.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/PreprocessorErrorTest.javacore/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.javacore/src/main/java/com/linecorp/armeria/internal/common/NoopCancellationScheduler.javacore/src/main/java/com/linecorp/armeria/internal/client/TailPreClient.javacore/src/test/java/com/linecorp/armeria/client/HttpPreprocessorTest.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsPreprocessor.javacore/src/main/java/com/linecorp/armeria/internal/common/DefaultCancellationScheduler.javacore/src/main/java/com/linecorp/armeria/internal/client/ClientUtil.javacore/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java
🧬 Code graph analysis (3)
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilter.java (1)
core/src/main/java/com/linecorp/armeria/client/UnprocessedRequestException.java (1)
UnprocessedRequestException(32-70)
grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcPreprocessorTest.java (2)
core/src/main/java/com/linecorp/armeria/client/Clients.java (1)
Clients(47-694)core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java (1)
DefaultClientRequestContext(105-1234)
core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java (1)
core/src/main/java/com/linecorp/armeria/common/util/UnmodifiableFuture.java (1)
UnmodifiableFuture(42-212)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-windows-latest-jdk-21
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: flaky-tests
- GitHub Check: site
- GitHub Check: build-macos-latest-jdk-21
- GitHub Check: lint
- GitHub Check: Kubernetes Chaos test
- GitHub Check: Summary
🔇 Additional comments (18)
core/src/main/java/com/linecorp/armeria/internal/client/ClientRequestContextExtension.java (1)
68-78: LGTM! Well-documented initialization lifecycle methods.The new
initAndFail()andinitializationTriggered()methods provide clear semantics for handling initialization failures and querying initialization state without blocking. The Javadoc clearly explains their purpose and fits well with the PR's goal of properly completing contexts during preprocessor failures.core/src/main/java/com/linecorp/armeria/internal/client/TailPreClient.java (1)
82-82: Verify the parameter change is intentional.The final boolean parameter to
initContextAndExecuteWithFallbackchanged fromtruetofalse. While this appears consistent with the broader initialization flow refactoring in this PR, please confirm this change is correct and aligns with the updated initialization semantics.xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsPreprocessor.java (1)
72-72: LGTM! Proper context cancellation on error.The addition of
ctx.cancel(e)before rethrowing ensures the context lifecycle completes properly when errors occur in the asynchronous path. This aligns with the PR objective of ensuringctx.whenInitialized()completes in failure scenarios.core/src/main/java/com/linecorp/armeria/internal/common/NoopCancellationScheduler.java (1)
120-123: LGTM! Correct no-op implementation.Returning
falseforhasEventLoop()is the appropriate behavior for a no-op cancellation scheduler and aligns with the interface contract.core/src/main/java/com/linecorp/armeria/client/retry/RetryingRpcClient.java (1)
175-175: LGTM! Cleaner initialization state check.The refactoring from endpoint null checks to
initializationTriggered()provides a more explicit and direct way to determine whether context initialization is needed for retry attempts. This aligns well with the new initialization lifecycle API introduced inClientRequestContextExtension.xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilter.java (1)
56-61: LGTM! Consistent error path cancellation.All four synchronous error paths correctly call
ctx.cancel(e)before throwing, ensuring proper context lifecycle completion. This aligns with the PR objective of properly completing contexts when requests fail at the preprocessor level.Also applies to: 65-69, 73-77, 96-100
core/src/main/java/com/linecorp/armeria/internal/common/DefaultCancellationScheduler.java (1)
276-279: LGTM!The
hasEventLoop()method correctly exposes event loop availability. SinceeventLoopis only set within lock-protected sections (init()at line 98-106), visibility is guaranteed through the lock's happens-before relationship with subsequent reads.core/src/main/java/com/linecorp/armeria/internal/client/ClientUtil.java (1)
179-191: Proper handling of initialization failure in PreClient execution.The changes correctly:
- Call
initAndFail(e)when an exception occurs before initialization is triggered, ensuringctx.whenInitialized()completes- Always invoke
completeLogIfIncomplete(ctx, res)after the try-catch block, ensuring consistent log completion for both success and failure pathsThis addresses the PR objective where exceptions in Preprocessor weren't completing
ctx.whenInitialized.grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcPreprocessorTest.java (2)
39-55: LGTM!The test correctly validates that when a preprocessor throws an exception:
- The exception is wrapped in
StatusRuntimeExceptionctx.whenInitialized()completes (isDone)ctx.log().isComplete()becomes truectx.eventLoop()is non-null (event loop was acquired)
57-77: LGTM!The test validates the scenario where
ctx.cancel()is called before initialization completes. The async execution viaCompletableFuture.supplyAsynccorrectly simulates a cancel occurring during response processing, verifying that the context properly handles cancellation before initialization.core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java (7)
120-123: Appropriate use of atomic updater for initialization tracking.The
AtomicIntegerFieldUpdaterwithvolatile int initializationTriggeredcorrectly implements thread-safe one-time initialization semantics. Using an int (0/1) instead of boolean is a common pattern withAtomicIntegerFieldUpdatersince there's noAtomicBooleanFieldUpdater.Also applies to: 142-143
366-369: LGTM!The CAS guard correctly prevents concurrent initialization attempts. If another thread already triggered initialization, we return the existing
whenInitialized()future rather than proceeding with duplicate initialization.
393-402: Core fix for the PR objective.This method correctly handles the case where a Preprocessor throws or
ctx.cancel()is called beforectx.init(). It:
- Atomically claims initialization responsibility via CAS
- Acquires an event loop (required for completion)
- Fails the request early with proper logging
- Completes
whenInitialized()with failureThis ensures gRPC can observe initialization failures, addressing the core bug.
496-511: Good defensive guard against double completion.The
isDone()check beforecomplete()is a reasonable defensive measure. WhileCompletableFuture.complete()is already idempotent (returns false if already completed), the explicit check makes the intent clear and avoids potential edge cases in the two-path logic (existing future vs. CAS-created future).
669-689: LGTM!The
hasEventLoop()guard correctly prevents re-initialization of the cancellation scheduler. This addresses the past review comment suggesting removal fromacquireEventLoop- now the method is idempotent and can be safely called multiple times.
656-667: Correct initialization for derived contexts.When creating a derived context with a known endpoint,
initNow()correctly marks initialization as triggered since no async endpoint selection is needed. The subsequentmaybeInitializeResponseCancellationScheduler()call properly initializes the cancellation scheduler after the event loop is acquired.
1078-1088: I'll help you verify the concern aboutfinishNow()idempotency in the cancellation scheduler. Let me search for the implementation.
<function_calls>
#!/bin/bashFind and examine finishNow() implementation in DefaultCancellationScheduler
rg -n -A 10 'void finishNow' core/src/main/java/com/linecorp/armeria/internal/common/DefaultCancellationScheduler.java
</function_calls>Let me also check the broader context of the DefaultCancellationScheduler to understand how it manages state:
<function_calls>
#!/bin/bashGet the full DefaultCancellationScheduler class
wc -l core/src/main/java/com/linecorp/armeria/internal/common/DefaultCancellationScheduler.java
</function_calls>Let me also verify the failEarly() method call path mentioned in the review:
<function_calls>
#!/bin/bashCheck the failEarly() method and its call to finishNow
rg -n -B 5 -A 5 'failEarly' core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java | head -50
</function_calls><end_of_turn>
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/PreprocessorErrorTest.java (1)
139-166: Unable to complete verification due to infrastructure constraints.The repository cloning failed persistently, preventing verification of the referenced
XdsResourceReader.fromYaml()method and the test file structure. Based on the original review comment requesting verification of this method and the test structure approval, manual verification is required to confirm:
- Availability and correct usage of
XdsResourceReader.fromYaml()- Presence of all required imports for classes used in the test
- Compliance with
@UnstableApiannotation requirements for any newly added public APIs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/common/CoreBlockHoundIntegration.java (1)
74-74: Consider groupingVersion.getAllwith similar one‑time/cached allowances or documenting rationaleThe new
allowBlockingCallsInside("com.linecorp.armeria.common.util.Version", "getAll")looks fine functionally and consistent with other BlockHound exemptions. To keep this list maintainable, it might be clearer to either:
- move it up under the existing “a single blocking call is incurred for the first invocation, but the result is cached” section (if that rationale applies), or
- add a short comment here explaining why blocking is acceptable for
getAll.This makes it easier to revisit exemptions later and ensure they remain justified as implementations evolve.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java(1 hunks)core/src/main/java/com/linecorp/armeria/client/retry/RetryingRpcClient.java(1 hunks)core/src/main/java/com/linecorp/armeria/common/CoreBlockHoundIntegration.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internal.- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/common/CoreBlockHoundIntegration.javacore/src/main/java/com/linecorp/armeria/client/retry/RetryingRpcClient.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-macos-latest-jdk-21
- GitHub Check: build-windows-latest-jdk-21
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: site
- GitHub Check: flaky-tests
- GitHub Check: lint
- GitHub Check: Kubernetes Chaos test
- GitHub Check: Summary
🔇 Additional comments (1)
core/src/main/java/com/linecorp/armeria/client/retry/RetryingRpcClient.java (1)
183-197: Initialization-triggered check is a good replacement for endpoint-based fallback gatingUsing
ctxExtension.initializationTriggered()to decide when to callinitContextAndExecuteWithFallbackcleanly decouples this fromEndpointGroup/endpoint-null checks while still ensuring we only re-init when the context hasn’t begun initialization. The null-guard onctxExtensionand restriction to non-initial attempts keep this safe and consistent with the new context lifecycle behavior described in the PR.
…ine#6466) Motivation: This issue was found while working on `GrpcServicesPreprocessor` refactoring. Currently, there is a bug where exceptions/response failures in `Preprocessor` do not complete `ctx.whenInitialized`. e.g. ``` WebClient.of((delegate, ctx, req) -> { // ctx.whenInitialized will not be completed throw new RuntimeException("error"); }).get("/"); ``` While normally this may not be an issue, gRPC relies on completion of this future to signal errors: ref: https://github.com/line/armeria/blob/1a31f3e8c9aba91fb663bfe5c4fc3eaa87c9d13f/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java#L180 I propose two modifications to resolve this issue: - When a Preprocessor throws an exception from the `PreClient#execute` calling thread, it should complete the ctx ``` WebClient.of((delegate, ctx, req) -> { throw new RuntimeException("error"); }).get("/"); ``` - When `ctx.cancel` is called (even before `ctx.init`) is called, it should complete the ctx ``` WebClient.of((delegate, ctx, req) -> { ctx.cancel(e); return HttpResponse.of(400); }).get("/"); ``` Modifications: - Introduced an `initialized` flag to guard against concurrent initialization attempts - `finishInitialization` is also modified to guard against concurrent calls - A new `initAndFail` method is introduced which acquires an event loop, initializes the cancellation scheduler, and completes the ctx - `ctx.cancel` will trigger `initAndFail` if the ctx is not initialized yet - If `PreClient#execute` throws an exception, `initAndFail` is called - For derived contexts, if an endpoint does not exist, it means `ClientUtil#initContextAnd*` needs to be called. Hence, initialization is set only if an endpoint exists. - `responseCancellationScheduler.finishNow` is called in `DefaultClientRequestContext#failEarly` to record `cancellationCause` consistently. - `XdsPreprocessor` and `RouterFilter` now calls `ctx.cancel` if a request is short-circuited before `ctx.init` is called. Result: - Requests failed at the `XdsPreprocessor`-level are propagated to the user correctly when using gRPC. <!-- Visit this URL to learn more about how to write a pull request description: https://armeria.dev/community/developer-guide#how-to-write-pull-request-description --> --------- Co-authored-by: Ikhun Um <ikhun.um@linecorp.com>
3 participants
Motivation:
This issue was found while working on
GrpcServicesPreprocessorrefactoring.Currently, there is a bug where exceptions/response failures in
Preprocessordo not completectx.whenInitialized.e.g.
While normally this may not be an issue, gRPC relies on completion of this future to signal errors:
ref:
armeria/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java
Line 180 in 1a31f3e
I propose two modifications to resolve this issue:
PreClient#executecalling thread, it should complete the ctxctx.cancelis called (even beforectx.init) is called, it should complete the ctxModifications:
initializedflag to guard against concurrent initialization attemptsfinishInitializationis also modified to guard against concurrent callsinitAndFailmethod is introduced which acquires an event loop, initializes the cancellation scheduler, and completes the ctxctx.cancelwill triggerinitAndFailif the ctx is not initialized yetPreClient#executethrows an exception,initAndFailis calledClientUtil#initContextAnd*needs to be called. Hence, initialization is set only if an endpoint exists.responseCancellationScheduler.finishNowis called inDefaultClientRequestContext#failEarlyto recordcancellationCauseconsistently.XdsPreprocessorandRouterFilternow callsctx.cancelif a request is short-circuited beforectx.initis called.Result:
XdsPreprocessor-level are propagated to the user correctly when using gRPC.