Fix ext_proc mock response handling in ext_proc client interceptor test

[kannanjgithub]

In ExternalProcessorClientInterceptorTest.clientInterceptor_contextPropagatedToStartCall(), the mock ext_proc service blindly returned a ProcessingResponse with setRequestHeaders for every incoming ProcessingRequest. When the downstream RPC finished and sent back response headers, the interceptor sent a ProcessingRequest with responseHeaders. The mock's blind requestHeaders reply caused the interceptor to detect an out-of-order protocol error, triggering internalOnError() which called delayedCall.cancel() on the executor thread.

This async cancellation on the executor thread raced with the test code's cleanup proxyCall.cancel() on the test runner thread, causing TSAN to report a data race on the non-volatile field ClientCallImpl.cancelCalled.

This commit fixes the test mock to check the request type (hasRequestHeaders() vs. hasResponseHeaders()) before responding, matching the behavior in clientInterceptor_contextPropagatedToListenerCallbacks().

Note: The unsynchronized cancellation between the application thread and an interceptor's cancellation on an executor thread, while technically a data race from TSAN's perspective due to ClientCallImpl.cancelCalled not being volatile, is not consequential in practice because the underlying stream.cancel() and CancellationHandler.tearDown() are thread-safe and idempotent. Therefore, modifying cancelCalled in ClientCallImpl is not necessary.

Internal TSAN test failure link

[ejona86]

is not consequential in practice because the underlying stream.cancel() and CancellationHandler.tearDown() are thread-safe and idempotent

But ClientCall.cancel() is not thread-safe. There could be another interceptor that has cancel() logic that would be broken. Also, ClientCallImpl.cancel() is not thread-safe because of cancelCalled. And it is purposefully not volatile because other methods throw after cancel is called; you can't use the ClientCall API safely from multiple threads.

[kannanjgithub]

That would mean we should not cancel the data plane RPC from within the interceptor. But as per A93 "If the stream terminates with a non-OK status, then by default the data plane RPC will be failed with INTERNAL status." Calling cancel is the only way to terminate the data plane RPC, and this will race with the application cancel, as it did in this test failure.

[ejona86]

That would mean we should not cancel the data plane RPC from within the interceptor.

It does not mean that at all. It only means that if you ignore all other options. Canceling the data plane RPC is a externally-visible behavior of the code, and is not something we do or not do because of a synchronization issue.

I don't know which specific cancel is the problem. But in general I'd expect us to do cancel the same way we handle messages; we already have to deal with synchronization for messages and the problem here should be very similar as they are simply different methods on the same non-thread-safe object.

[kannanjgithub]

We can simply synchronize the DataPlaneClientCall.cancel from the application and the DelayedClientCall.cancel in internalOnError handling using synchronized or AtomicBoolean. I will add that fix.