New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 650438 link

Starred by 1 user

Issue metadata

Status: Fixed
Merged: issue 603182
Owner:
Last visit > 30 days ago
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Cronet treats RST_STREAM with code NO_ERROR as a SPDY failure (error -337)

Project Member Reported by mxyan@google.com, Sep 26 2016

Issue description

Chrome Version       : Cronet transport for gRPC
URLs (if applicable) : N/A
Other browsers tested: N/A
  Add OK or FAIL, along with the version, after other browsers where you
have tested this issue:
     Safari:
    Firefox:
         IE:

What steps will reproduce the problem?
(1) N/A
(2)
(3)

What is the expected result?
Treats this RST_STREAM as an indication of server being in half-close state, but not a stream failure.

What happens instead?
on_failed() callback is invoked and messages are discarded.

Please provide any additional information below. Attach a screenshot if
possible.
When a server enters half-close state before a client sends end of stream, the server may send a RST_STREAM to the client with error code NO_ERROR. This RST is not to be treated as an error or failure according to RFC 7540. Cronet treats it as a failure now and calls on_failed() callback provided by gRPC. We hope the behavior could be adjusted to be a bit more graceful.
 

Comment 1 by eroman@chromium.org, Sep 26 2016

Components: Internals>Network>Library
Components: Internals>Network>HTTP2
Status: Available (was: Unconfirmed)
+ Internals>Network>HTTP2

Looks like we should handle this case. 

For the record, I think this is the quote that the OP is referring from RFC 7540:

   "An HTTP response is complete after the server sends -- or the client
   receives -- a frame with the END_STREAM flag set (including any
   CONTINUATION frames needed to complete a header block).  A server can
   send a complete response prior to the client sending an entire
   request if the response does not depend on any portion of the request
   that has not been sent and received.  When this is true, a server MAY
   request that the client abort transmission of a request without error
   by sending a RST_STREAM with an error code of NO_ERROR after sending
   a complete response (i.e., a frame with the END_STREAM flag).
   Clients MUST NOT discard responses as a result of receiving such a
   RST_STREAM, though clients can always discard responses at their
   discretion for other reasons."

Project Member

Comment 3 by sheriffbot@chromium.org, Sep 27 2016

Labels: Hotlist-Google

Comment 4 by b...@chromium.org, Sep 27 2016

Mergedinto: 603182
Status: Duplicate (was: Available)

Comment 5 by mxyan@google.com, Sep 27 2016

@xunjieli you are right that quote is exactly what I was referring to in RFC 7540.
Labels: -Pri-3 Pri-2
Status: Available (was: Duplicate)
kapishnikov@ pointed out that we need to add some handling on top of Bence's change(https://crrev.com/2445113002) in Cronet layer to notify client of the stream being closed so that client can abort any pending write.

De-duplicating this issue to track Cronet side work.
Cc: kapishnikov@chromium.org
Owner: xunji...@chromium.org
Status: Started (was: Available)
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f8b6bb6105a7265a4a93e2ed4fa05c72b84761a

commit 4f8b6bb6105a7265a4a93e2ed4fa05c72b84761a
Author: xunjieli <xunjieli@chromium.org>
Date: Mon Oct 31 23:16:00 2016

Make net::BidirectionalStream handle RST_STREAM_NO_ERROR.

This CL makes net::BidirectionalStream handle
RST_STREAM_NO_ERROR, so that future SendData() call will not
produce OnFailed() callback. This CL essentially make
net::BidirectionalStream blackhole SendData() after
receiving a RST_STREAM with no error.

BUG= 650438 

Review-Url: https://codereview.chromium.org/2462463002
Cr-Commit-Position: refs/heads/master@{#428858}

[modify] https://crrev.com/4f8b6bb6105a7265a4a93e2ed4fa05c72b84761a/net/spdy/bidirectional_stream_spdy_impl.cc
[modify] https://crrev.com/4f8b6bb6105a7265a4a93e2ed4fa05c72b84761a/net/spdy/bidirectional_stream_spdy_impl.h
[modify] https://crrev.com/4f8b6bb6105a7265a4a93e2ed4fa05c72b84761a/net/spdy/bidirectional_stream_spdy_impl_unittest.cc

Cc: mef@chromium.org
Status: Fixed (was: Started)
Many thanks to kapishnikov@ who suggested the fix and reviewed the CL. 
mxyan@: could you try again with the fix?

Comment 10 by mef@chromium.org, Nov 1 2016

I'll share a new build with mxyan@ soon.

Comment 11 by mxyan@google.com, Nov 1 2016

Thank you! I will give a try here.

Comment 12 by mxyan@google.com, Nov 2 2016

Working well. Thanks bnc@ xunjieli@ kapishnikov@ mef@

Sign in to add a comment