core,netty: client sends rst stream when server half-closes #4222
Conversation
|
|
||
| CancelClientStreamCommand(NettyClientStream.TransportState stream, Status reason) { | ||
| this.stream = Preconditions.checkNotNull(stream, "stream"); | ||
| Preconditions.checkNotNull(reason, "reason"); | ||
| Preconditions.checkArgument(!reason.isOk(), "Should not cancel with OK status"); | ||
| Preconditions.checkArgument( |
There was a problem hiding this comment.
Im a little uncomfortable with using null here. I think a sentinel value like Status.CANCELED.withDescription("early trailers") would be a safer approach. Anything else that accesses the reason may expect it to be non null. Also, most of gRPC makes the status non null.
There was a problem hiding this comment.
The point of null here is to be able to send RST_STREAM to the server without invoking transportReportStatus (again) in NettyClientHandler#cancelStream. This field is accessed in exactly one place (NettyClientHandler#cancelStream, which should be the only thing parsing these commands anyways?), and it checks for null to decide whether to report the status to the transport. I could accomplish the same by adding a RstStreamCommand (and had this in an earlier draft of this PR), but it would be functionally almost identical to CancelClientStreamCommand so seemed like unnecessary redundancy.
ejona86
added
the
TODO:release blocker
This is actually the intended behavior. To do something smarter needs synchronization and only really helps clients who aren't paying attention. This case is more common for |
| @@ -293,6 +298,9 @@ public void deframeFailed(Throwable cause) { | |||
|
|
|||
| void transportHeadersReceived(Http2Headers headers, boolean endOfStream) { | |||
| if (endOfStream) { | |||
| if (!isOutboundClosed()) { | |||
| handler.getWriteQueue().enqueue(new CancelClientStreamCommand(this, null), true); | |||
There was a problem hiding this comment.
If you do this after the transportTrailersReceived() call, then there should be no need to avoid the second call to transportReportStatus; we call transportReportStatus lots of times on failures and that's okay. It gets too complicated trying to avoid it.
The Status used here should probably make it obvious that it shouldn't be seen though. Maybe: Status.INTERNAL.withDescription("Cancelled stream after server closed. The server's status should be reported instead of this one")
There was a problem hiding this comment.
Shoot. I am wrong. The second call to transportReportStatus() will have stopDelivery=true. The call from transportTrailersReceived() will have stopDelivery=false. Let's put it back to the way you had it.
| @@ -280,6 +280,11 @@ public void runOnTransportThread(final Runnable r) { | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public void deframerClosed(boolean hasPartialMessageIgnored) { | |||
| @@ -175,8 +174,8 @@ public final void deliverFrame( | |||
|
|
|||
| @Override | |||
| public final void halfClose() { | |||
| if (!outboundClosed) { | |||
| outboundClosed = true; | |||
| if (!transportState().isOutboundClosed()) { | |||
There was a problem hiding this comment.
I'd rather this conditional remain like it was (non-volatile). As it is written here, it seems like some other code path could setOutboundClosed(). However, it actually appears to be an unnecessary condition (halfClose is only called once), so let's instead leave it as you have it and do a follow-up PR to remove the condition.
ejona86
removed
the
TODO:release blocker
This addresses #4202. I tried to keep the changes minimal, to allow for easily backporting this to earlier releases if necessary.
Currently, if a server half-closes a stream with a Netty client, both server and client will hold onto the stream resources until (if ever) the client calls halfClose(). The same problem does not occur with an OkHttp client, as
OkHttpClientTransport.TransportState#onEndOfStreamtriggers deletion of the stream reference on the client and sends a RST_STREAM to the server, which lets the Netty server drop the stream resources as well.This PR changes the Netty client behavior to be similar to that of OkHttp clients: when end of stream is received from the server, the client sends a RST_STREAM if it has not already half-closed its side of the stream. OkHttp currently waits to do this until the deframer closes, which does not seem to be necessary: anything the client sends at this point will be ignored by the server, so we might as well close the stream as early as possible. This PR doesn't change the OkHttp behavior; that can be in a follow-up PR.
Sending the reset stream releases the stream resources in Netty due to triggering
Http2ConnectionHandler#processRstStreamWriteResult, which ends up invokingDefaultHttp2Connection.ActiveStreams#deactivate, which removes the stream from the list of active streams. Without sending the reset stream, all of the client's streams remain active and a reference is retained to each.We also almost certainly want to close the framer, if it isn't already closed, when the end of stream is seen from the server (again, this is left to a follow-up PR). Currently, the framer is only ever closed if the client invokes
halfClose()on the stream. For both Netty and OkHttp, the client may continue to invokeClientCall#sendMessage()even after the server half-closes, and the message goes all the way through the framer, even being reported as outbound bytes to the stats tracers, before ultimately being stopped at the transport layer before going out on the now-dead stream: for Netty, this happens in DefaultHttp2ConnectionEncoder and for OkHttp it happens in OutboundFlowController, but it would be much cleaner and more efficient to close the framer as soon as we know the stream was closed by the server.There is likely still follow-up work to do on the server side to drop stream resources even when the client does not send the reset stream; this PR only changes client-side behavior.