xds: Fix TSAN data race on ClientCall cancellation in ExternalProcessorClientInterceptor - #13022
Open
kannanjgithub wants to merge 3 commits into
Open
xds: Fix TSAN data race on ClientCall cancellation in ExternalProcessorClientInterceptor#13022kannanjgithub wants to merge 3 commits into
kannanjgithub wants to merge 3 commits into
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
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
1 participant
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.