xds: Fix TSAN data race on ClientCall cancellation in ExternalProcessorClientInterceptor - #13022
Merged
Merged
Conversation
…orClientInterceptor Prevent concurrent cancellations of the underlying ClientCall in ExternalProcessorClientInterceptor: - Wrap rawCall with SimpleForwardingClientCall using an AtomicBoolean to ensure the underlying ClientCall.cancel() is executed at most once, even if invoked concurrently across threads or from DelayedListener. - Remove redundant downstreamCancelled AtomicBoolean from DataPlaneClientCall and simplify cancelDownstream() to directly delegate to delayedCall.cancel(), as DelayedClientCall internally synchronizes pending cancellations and the wrapped rawCall deduplicates active cancellations. - Remove the now-unused rawCall field and constructor parameter from DataPlaneListener. - In DataPlaneClientCall.cancel() and validateCompressionSupport(), atomically transition extProcStreamState to FAILED and clear extProcClientCallRequestObserver. - In sendToExtProc(), return early if the ext-proc stream is already completed or the observer is null. - Safely complete and clear extProcClientCallRequestObserver in closeExtProcStream(). - Route all rawCall.cancel() calls in sendMessage(), handleImmediateResponse(), and DataPlaneListener through cancelDownstream(). Jetski conversations: - 502db5da-b88e-474c-9c05-a14b441d3eac - 3318c948-f0f7-49ce-afe3-5670e30bf376 CONV=502db5da-b88e-474c-9c05-a14b441d3eac
# Conflicts: # xds/src/main/java/io/grpc/xds/ExternalProcessorClientInterceptor.java
sauravzg
approved these changes
Sep 7, 2026
sauravzg
left a comment
sauravzg
left a comment
Contributor
There was a problem hiding this comment.
Can we add a separate unit test that checks some form of concurrent cancellation?
…ientInterceptor Add unit test verifying that when a call is cancelled concurrently from multiple threads while a data plane response is arriving: - The underlying ClientCall.cancel() is executed exactly once, verifying deduplication and thread-safety of the wrapped rawCall. - The external processor stream observer receives onError at most once, and no requests are sent to ext-proc after cancellation. Jetski conversations: - 502db5da-b88e-474c-9c05-a14b441d3eac - 3318c948-f0f7-49ce-afe3-5670e30bf376 CONV=502db5da-b88e-474c-9c05-a14b441d3eac
Contributor
Author
Added. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prevent concurrent cancellations of the underlying ClientCall in ExternalProcessorClientInterceptor:
Fixes the TSAN detected race condition below. The previous fix in commit cf92f2d introduced
AtomicBoolean downstreamCancelledincancelDownstream(). However, the TSAN data race still reproduced under postsubmit becauseDelayedListenerexception path bypassescancelDownstream(): WhenDelayedClientCall.start(wrappedListener, headers)is called, it registers an internalDelayedListener. IfwrappedListener.onHeaders()throws any exception,DelayedListener.deliverHeaders()catches it and callsexceptionThrown(), which invokesdelayedCall.cancel()directly. OncedelayedCallhas been activated(realCall != null),DelayedClientCall.cancel()directly callsrealCall.cancel()on whatever thread triggered it without any de-duplication.