core: Made ServerImpl.internalClose thread-safe. #11864
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. |
| @@ -503,7 +503,7 @@ private void streamCreatedInternal( | |||
|
|
|||
| 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.
| @@ -503,7 +503,7 @@ private void streamCreatedInternal( | |||
|
|
|||
| 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.
| @@ -808,10 +824,9 @@ void setListener(ServerStreamListener listener) { | |||
| /** | |||
| * Like {@link ServerCall#close(Status, Metadata)}, but thread-safe for internal use. | |||
| */ | |||
| 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).
core: Added changes to make ServerImpl.internalClose thread-safe and trigger cancel instead of completed.
Fixes #3746.