core: Made ServerImpl.internalClose thread-safe.#11864
core: Made ServerImpl.internalClose thread-safe.#11864harshagoo94 wants to merge 14 commits intogrpc:masterfrom
Conversation
|
Note: We are not following cancel calls as 2nd option (since option 2 is not feasible). Instead, we are allowing truncated messages in the stream and delivering the trailers. |
| final JumpToApplicationThreadServerStreamListener jumpListener | ||
| = new JumpToApplicationThreadServerStreamListener( | ||
| wrappedExecutor, executor, stream, context, tag); | ||
| wrappedExecutor, executor, stream, context, tag, headers); |
There was a problem hiding this comment.
You need to create the trailers with metadata from the exception caught. See example.
There was a problem hiding this comment.
I agree that using these headers is very wrong, as echoing back the client's request headers is harmful. Although, in fact, the original code was fine and it should just be a new set of Metadata. None of the callers of internalClose() has metadata attached, and even if they do, we'd need to understand why a bit better because sending RST_STREAM was a valid way to handle this, which won't have metadata.
| final JumpToApplicationThreadServerStreamListener jumpListener | ||
| = new JumpToApplicationThreadServerStreamListener( | ||
| wrappedExecutor, executor, stream, context, tag); | ||
| wrappedExecutor, executor, stream, context, tag, headers); |
There was a problem hiding this comment.
I agree that using these headers is very wrong, as echoing back the client's request headers is harmful. Although, in fact, the original code was fine and it should just be a new set of Metadata. None of the callers of internalClose() has metadata attached, and even if they do, we'd need to understand why a bit better because sending RST_STREAM was a valid way to handle this, which won't have metadata.
| */ | ||
| private void internalClose(Throwable t) { | ||
| // TODO(ejona86): this is not thread-safe :) | ||
| private synchronized void internalClose(Throwable t) { |
There was a problem hiding this comment.
Throwing synchronized on it doesn't make it thread-safe. You'd have to synchronize most calls to the stream, and make sure to stop calling the stream after closing it. And we don't want to synchronize most calls to the stream. We will need help from the stream to implement this.
There was a problem hiding this comment.
Since all 3 callers of ServerImpl.closeInternal viz., onReady, messagesAvailable and halfClosed run serialized on the callExecutor, what makes the stream.close call in internalClose non-thread safe? Is the race with other methods on the stream?
There was a problem hiding this comment.
onReady, messagesAvailable, and halfClosed are callbacks and don't generally call into stream (other than internalClose()). You need to look for other calls into stream. That would be from ServerCallImpl which is called by the application on arbitrary threads (but only one thread concurrently).
This is the "three threads" we had talked about for RPCs: application thread, transport thread, callback thread. stream is used on the application thread, yet here we are using it from the callback thread (callExecutor).
|
Created the new PR with required changes and Addressed Review points in #11924. we will close this PR as duplicate once a new one merged. |
|
You could have added your commit on top of this PR itself. But it's fine now. |
I tried that and mentioned to you earlier in the tvc group as its Harsha's fork and don't have permission to push the changes. hence created the new branch/PR. |
|
Can we close this already if we are going to work with the new PR? |
Yes Kannan we can close this as a duplicate of PR #11924 which was raised earlier by Harsha. Even unable to close this PR as its Harsha's fork. Request you to close the same if you are able to see the same option or any other way to close it from the backend? |
5 participants
core: Added changes to make ServerImpl.internalClose thread-safe and trigger cancel instead of completed.
Fixes #3746.