Repository navigation
fix(grpc): propagate invocation cancellation to the transport - #8078
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Review (Aias00)
Verdict: APPROVE. I reproduced both directions: the new suite is green on the head and fails 6/10 when the production hunk is reverted, so the tests actually pin the behavior. The fix also stays inside the lifecycle it touches rather than restructuring the client.
What I verified
mvn -pl shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-rpc/shenyu-plugin-grpc -am test: 90 tests, 0 failures, 0 errors, 0 skipped, includingShenyuGrpcCancellationTest10/10.- Reverted only the
AtomicBoolean+ForwardingClientCall+ listener hunk (leaving the tests untouched) and re-ran:Tests run: 10, Failures: 6—cancellationBeforeResponseReachesTransport(all fourMethodTypes),cancellationDuringStreamingReachesTransport,reactorCancellationReachesTransport. That matches the PR description exactly, and the six are the right six: they are the ones that assert transport cancellation rather than the future's own flag. - The pre-existing
ShenyuGrpcClientTeststill passes 4/4, so the mock-based test was not deleted to make room for the new one — it is still there, now backed by a real-transport suite next to it.
The mechanism is right, and narrow
invoke()returnsdoneObserver.getCompletionFuture(), andcall()cancels that future viaresult.whenComplete. Registering adirectExecutorlistener on that same future is the minimal way to bridge "future cancelled" toClientCall.cancel, and it lands on the existing object rather than introducing a second lifecycle.completion.isCancelled()is the correct guard: normal completion and upstream failure both complete the future, so neither fires an extra cancel.normalCompletionDoesNotCancelTransportandupstreamFailureDoesNotCancelTransportAgainassert exactly that (0 cancellations), which is the regression I would have expected this change to introduce.- Sharing the
AtomicBoolean-guarded wrapper withMessageWriter.bindCall(call)is the right call: aggregate overflow and downstream cancellation now converge on one cleanup, andoverflowAndDownstreamCancellationShareTransportCleanupproves it by asserting the count stays at exactly 1 across both triggers. Before this, overflow held the raw call while the invocation future held nothing. ForwardingClientCall.SimpleForwardingClientCallkeepsstart/sendMessage/halfCloseuntouched, so no behavior changes for the four RPC modes beyond cancellation.
Nits (non-blocking)
completionRacingCancellationCleansTransportAtMostOnceassertscall.cancellations.get() <= 1, which is satisfied by 0 as well — the assertion cannot fail on its own.assertTrue(result.isDone())is doing all the work in that loop. If the intent is "never double-clean", pair it with an invariant that actually holds under both outcomes, e.g. assert thatcancellations == 1exactly when the cancel branch won, or record which future completed first and assert the count matches that branch.- The wrapper drops a second
cancelcall outright, including its message/cause. For overflow-then-downstream-cancel that is fine, but if the first cancellation carries a generic cause and a later one is more specific, the transport only ever sees the first. Worth a one-line comment stating that the first cancellation wins by design, so the next reader does not "fix" it into a message overwrite. invoke()still returnsnullfor an unknownMethodTypewhile having already registered the listener oncompletion. Pre-existing, not introduced here, but the new code makes the null-return path slightly more visible; athrowwould be friendlier than a NPE three frames later. Separate change, not this PR.
Merge-order note
PR #8035 (fix(grpc): drain client and bidi streams according to outbound readiness) touches the same client-streaming/bidi paths. Both can land, but whichever merges second will need to re-run the other's suite: this change adds a cancel wrapper around the call that #8035's drain logic will also hold. Worth coordinating so the second one is not merged on a stale green.
CI
Green on 3fd18968: ci / build / build (shenyu-integrated-test-grpc) / build (shenyu-integrated-test-k8s-ingress-grpc) / build (shenyu-integrated-test-k8s-gateway-api-http) / e2e-case (e2e-grpc-sync-compose ...) / CodeQL Analyze (java) / pr_build / rat_check / check-license-header / registry_dependencies / build-docker-images all pass.
Fixes #8020.
Cancelling the gateway result currently cancels only the independent invocation future. Bind that future's cancellation to the actual ClientCall, and share an idempotent transport cancellation wrapper with aggregate-overflow cleanup. Normal completion and upstream failure do not introduce an extra cancellation.
Validation on OpenJDK 17.0.20:
These tests use a recording ClientCall, not a deployed gRPC server. Full-repository and Docker E2E suites were not run. Local Mockito tests use the existing Byte Buddy agent because self-attachment is unavailable. The separate outbound-readiness work in PR #8035 may need integration when either change lands.