Retry request upon GOAWAY frame with NO_ERROR.

Retry request upon GOAWAY frame with NO_ERROR.

Retry request upon receiving a GOAWAY frame with error code NO_ERROR
and with Last-Stream-ID lower than stream id corresponding to request.

BUG=681477
Review-Url: https://codereview.chromium.org/2820163004
Cr-Commit-Position: refs/heads/master@{#465717}
Committed: https://chromium.googlesource.com/chromium/src/+/c4769d412d1b89724db8eb42577cd975fd85f559

[Bence, 2017-04-18 16:49:20]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 16:49:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bence, 2017-04-18 19:01:02]

bnc@chromium.org changed reviewers:
+ zhongyi@chromium.org

[Bence, 2017-04-18 19:01:02]

Zhongyi: PTAL.  Thank you.

[commit-bot: I haz the power, 2017-04-18 19:12:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 19:12:18]

Dry run: This issue passed the CQ dry run.

[Zhongyi Shi, 2017-04-19 00:40:28]

It's always cool to move more close to the RFC!

https://codereview.chromium.org/2820163004/diff/1/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/2820163004/diff/1/net/base/net_error_list.h#n...
net/base/net_error_list.h:649: // Client MAY retry (on a different connection). 
See RFC7540 Section 8.1.4.
optional nit: there're two spaces before "See RFC7540...". Is that on purpose?

https://codereview.chromium.org/2820163004/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2820163004/diff/1/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:2698: } else if (error_code == ERROR_CODE_NO_ERROR) {
I am not familiar with SPDY code, but according to your CL description: should
we evaluate the Last-Stream-ID vs stream id corresponding to the request before
populating the ERR_SPDY_SERVER_REFUSED_STREAM? Or is that already handled
somewhere else, just not present in this CL?

That also suggests a unit test in spdy_network_transaction_unittest.cc where the
GO_AWAY_FRAME is received with ERROR_CODE_NO_ERROR but inappropriate stream id,
test that won't cause us to retry on a new connection.

[Bence, 2017-04-19 12:12:16]

Thank you for your comments.  PTAL.

https://codereview.chromium.org/2820163004/diff/1/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/2820163004/diff/1/net/base/net_error_list.h#n...
net/base/net_error_list.h:649: // Client MAY retry (on a different connection). 
See RFC7540 Section 8.1.4.
On 2017/04/19 00:40:27, Zhongyi Shi wrote:
> optional nit: there're two spaces before "See RFC7540...". Is that on purpose?

I usually write two spaces between sentences by default, but do not have strong
feelings about it and am not aware of any guidance in any of our style guides. 
I am leaving this for the time being because you said "optional", but I am happy
to change it to a single space if you prefer.

https://codereview.chromium.org/2820163004/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2820163004/diff/1/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:2698: } else if (error_code == ERROR_CODE_NO_ERROR) {
On 2017/04/19 00:40:28, Zhongyi Shi wrote:
> I am not familiar with SPDY code, but according to your CL description: should
> we evaluate the Last-Stream-ID vs stream id corresponding to the request
before
> populating the ERR_SPDY_SERVER_REFUSED_STREAM? Or is that already handled
> somewhere else, just not present in this CL?
> 
> That also suggests a unit test in spdy_network_transaction_unittest.cc where
the
> GO_AWAY_FRAME is received with ERROR_CODE_NO_ERROR but inappropriate stream
id,
> test that won't cause us to retry on a new connection. 

Excellent questions.

In StartGoingAway() there is a loop [1] that calls LogAbandonedActiveStream only
for active streams with stream_id exceeding Last-Stream-ID.  Active streams with
lower stream_ids are kept as active, because the server is possibly still
sending data on them.

As for the unittest, this is what SpdyNetworkTransactionTest.GracefulGoaway [2]
does: |goaway| is the GOAWAY frame with a very high Last-Stream-ID value, and
the second request is accepted on the same connection after receiving this
frame.

[1]
https://cs.chromium.org/chromium/src/net/spdy/spdy_session.cc?q=startgoingawa...
[2]
https://cs.chromium.org/chromium/src/net/spdy/spdy_network_transaction_unitte...

[Zhongyi Shi, 2017-04-19 17:19:18]

lgtm!

https://codereview.chromium.org/2820163004/diff/1/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/2820163004/diff/1/net/base/net_error_list.h#n...
net/base/net_error_list.h:649: // Client MAY retry (on a different connection). 
See RFC7540 Section 8.1.4.
On 2017/04/19 12:12:15, Bence wrote:
> On 2017/04/19 00:40:27, Zhongyi Shi wrote:
> > optional nit: there're two spaces before "See RFC7540...". Is that on
purpose?
> 
> I usually write two spaces between sentences by default, but do not have
strong
> feelings about it and am not aware of any guidance in any of our style guides.

> I am leaving this for the time being because you said "optional", but I am
happy
> to change it to a single space if you prefer.

Acknowledged.

https://codereview.chromium.org/2820163004/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2820163004/diff/1/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:2698: } else if (error_code == ERROR_CODE_NO_ERROR) {
On 2017/04/19 12:12:15, Bence wrote:
> On 2017/04/19 00:40:28, Zhongyi Shi wrote:
> > I am not familiar with SPDY code, but according to your CL description:
should
> > we evaluate the Last-Stream-ID vs stream id corresponding to the request
> before
> > populating the ERR_SPDY_SERVER_REFUSED_STREAM? Or is that already handled
> > somewhere else, just not present in this CL?
> > 
> > That also suggests a unit test in spdy_network_transaction_unittest.cc where
> the
> > GO_AWAY_FRAME is received with ERROR_CODE_NO_ERROR but inappropriate stream
> id,
> > test that won't cause us to retry on a new connection. 
> 
> Excellent questions.
> 
> In StartGoingAway() there is a loop [1] that calls LogAbandonedActiveStream
only
> for active streams with stream_id exceeding Last-Stream-ID.  Active streams
with
> lower stream_ids are kept as active, because the server is possibly still
> sending data on them.
> 
> As for the unittest, this is what SpdyNetworkTransactionTest.GracefulGoaway
[2]
> does: |goaway| is the GOAWAY frame with a very high Last-Stream-ID value, and
> the second request is accepted on the same connection after receiving this
> frame.
> 
> [1]
>
https://cs.chromium.org/chromium/src/net/spdy/spdy_session.cc?q=startgoingawa...
> [2]
>
https://cs.chromium.org/chromium/src/net/spdy/spdy_network_transaction_unitte...

Ah, I see, thanks for the clarification and code walkthrough!

[Bence, 2017-04-19 17:42:31]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2017-04-19 17:42:32]

Ryan: PTAL.
Zhongyi: Thank you for reviewing.

[Ryan Hamilton, 2017-04-19 17:47:08]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2017-04-19 17:47:08]

lgtm

[commit-bot: I haz the power, 2017-04-19 17:47:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 19:57:58]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1492624028065870, "parent_rev":
"28ac2d11e80db21b5b7a37968d558c7e95897ebf", "commit_rev":
"c4769d412d1b89724db8eb42577cd975fd85f559"}

[commit-bot: I haz the power, 2017-04-19 19:58:04]

Description was changed from

==========
Retry request upon GOAWAY frame with NO_ERROR.

Retry request upon receiving a GOAWAY frame with error code NO_ERROR
and with Last-Stream-ID lower than stream id corresponding to request.

BUG=681477
==========

to

==========
Retry request upon GOAWAY frame with NO_ERROR.

Retry request upon receiving a GOAWAY frame with error code NO_ERROR
and with Last-Stream-ID lower than stream id corresponding to request.

BUG=681477

Review-Url: https://codereview.chromium.org/2820163004
Cr-Commit-Position: refs/heads/master@{#465717}
Committed:
https://chromium.googlesource.com/chromium/src/+/c4769d412d1b89724db8eb42577c...
==========

[commit-bot: I haz the power, 2017-04-19 19:58:05]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/c4769d412d1b89724db8eb42577c...