Is there a reason that ERR_QUIC_HANDSHAKE_FAILED retry logic is not encapsulated in QuicHttpStream?

We can put the retry logic in net::BidirectionalStream which wraps BidirectionalStreamQuicImpl and BidirectionalStreamSpdyImpl. But if the retry logic can be handled at the same place for both Http and bidi (maybe in QuicHttpStream?), then that will be better.

Good question. Yes, there is a specific reason and a general reason.

Specifically, the QuicHttpStream has a handle to a single session, but the session has closed with an error, so if we wanted to retry the request, we can't use the existing session (because it's closed) and we have no other place to send it :(

But more generally, the network transaction is the part of the HTTP stack which sits on top of the HttpStreamFactory and it is the HttpStreamFactory which is required to get a new HttpStream for the retried request. In addition to handling ERR_QUIC_HANDSHAKE_FAILED, HttpNetworkTransaction has retry logic for a whole variety of different types of network errors (including SSL client auth, HTTP auth, etc). So I think it makes sense, for HTTP, to do the retry there.

This works in HTTP land, because the caller interacts via a URLRequests not with the HttpStream directly, so it's possible to transparently swap out the underlying HttpStream. I guess since cronet callers don't have a layer of indirection here, that'd require a new layer to be introduced?

I am not sure if we can reliably retry a request at Cronet's bidi layer.
Say a consumer does 0-RTT and sends some data with request headers, and that consumer gets an ERR_QUIC_HANDSHAKE_FAILED. The problem now with retry is that Cronet doesn't buffer that write data internally. It doesn't know how to rewind (unlike Http's net::UploadDataStream). 

Doh! If we wanted to mitigate this case, we could have cronet know when it's sending 0-RTT data and buffer in that case (but not if the handshake is already confirmed)? If cronet doesn't buffer the data, and the app does not buffer the data, is that going to result in a user-visible failure? 

Yea, if we want Cronet to retry requests for 0RTT Posts/Puts when handshake is not confirmed, we need to make Cronet buffer the data beforehand. With bidi stream, it is tricky because data can be sent/read when the write side is not closed. There is no request/response semantics. It's unclear how much send data Cronet should buffer.

I have been suggesting to Cronet consumers to implement their own retry logic (b/37833240#comment4).
We can make ERR_QUIC_HANDSHAKE_FAILED as a retry-able failure here https://cs.chromium.org/chromium/src/components/cronet/android/java/src/org/chromium/net/impl/NetworkExceptionImpl.java?rcl=e5d5eef46f948ec1aa28739ce6fda52e6a673f68&l=44.


Is that a good enough mitigation strategy?

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

commit 72fd1d6322d691ea21535a739546125f6364ce05
Author: Helen Li <xunjieli@chromium.org>
Date: Fri Aug 25 14:02:39 2017

[cronet] Add two more immediatelyRetryable error codes

- adds a new BidirectionalStreamNetworkException class.
- adds net::ERR_SPDY_PING_FAILED and net::ERR_QUIC_HANDSHAKE_FAILED
  to BidirectionalStreamNetworkException's immediatelyRetryable error
  codes.

Bug:  757523 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: I01482dd07a43b1fc404dd7943b7ae4242631e7b2
Reviewed-on: https://chromium-review.googlesource.com/630139
Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497396}
[modify] https://crrev.com/72fd1d6322d691ea21535a739546125f6364ce05/components/cronet/android/BUILD.gn
[modify] https://crrev.com/72fd1d6322d691ea21535a739546125f6364ce05/components/cronet/android/api.txt
[modify] https://crrev.com/72fd1d6322d691ea21535a739546125f6364ce05/components/cronet/android/api/src/org/chromium/net/NetworkException.java
[modify] https://crrev.com/72fd1d6322d691ea21535a739546125f6364ce05/components/cronet/android/api_version.txt
[add] https://crrev.com/72fd1d6322d691ea21535a739546125f6364ce05/components/cronet/android/java/src/org/chromium/net/impl/BidirectionalStreamNetworkException.java
[modify] https://crrev.com/72fd1d6322d691ea21535a739546125f6364ce05/components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java
[modify] https://crrev.com/72fd1d6322d691ea21535a739546125f6364ce05/components/cronet/android/java/src/org/chromium/net/impl/NetworkExceptionImpl.java
[modify] https://crrev.com/72fd1d6322d691ea21535a739546125f6364ce05/components/cronet/android/url_request_error.cc
[modify] https://crrev.com/72fd1d6322d691ea21535a739546125f6364ce05/components/cronet/android/url_request_error.h

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

commit 7b4b0d1feadff5ac989f0a73f190a35c08cc0a07
Author: Helen Li <xunjieli@chromium.org>
Date: Mon Aug 28 16:11:05 2017

Revert "[cronet] Add two more immediatelyRetryable error codes"

This reverts commit 72fd1d6322d691ea21535a739546125f6364ce05.

Reason for revert: <Might break embedders>

Original change's description:
> [cronet] Add two more immediatelyRetryable error codes
> 
> - adds a new BidirectionalStreamNetworkException class.
> - adds net::ERR_SPDY_PING_FAILED and net::ERR_QUIC_HANDSHAKE_FAILED
>   to BidirectionalStreamNetworkException's immediatelyRetryable error
>   codes.
> 
> Bug:  757523 
> Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
> Change-Id: I01482dd07a43b1fc404dd7943b7ae4242631e7b2
> Reviewed-on: https://chromium-review.googlesource.com/630139
> Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org>
> Commit-Queue: Helen Li <xunjieli@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#497396}

TBR=xunjieli@chromium.org,kapishnikov@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  757523 
Change-Id: I11f5fa2044f54e9f0f129b7037f101a8be3e3e19
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Reviewed-on: https://chromium-review.googlesource.com/638430
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497770}
[modify] https://crrev.com/7b4b0d1feadff5ac989f0a73f190a35c08cc0a07/components/cronet/android/BUILD.gn
[modify] https://crrev.com/7b4b0d1feadff5ac989f0a73f190a35c08cc0a07/components/cronet/android/api.txt
[modify] https://crrev.com/7b4b0d1feadff5ac989f0a73f190a35c08cc0a07/components/cronet/android/api/src/org/chromium/net/NetworkException.java
[modify] https://crrev.com/7b4b0d1feadff5ac989f0a73f190a35c08cc0a07/components/cronet/android/api_version.txt
[delete] https://crrev.com/b1f1bb311d82c510f49c19fabc67e81a75e5d6b0/components/cronet/android/java/src/org/chromium/net/impl/BidirectionalStreamNetworkException.java
[modify] https://crrev.com/7b4b0d1feadff5ac989f0a73f190a35c08cc0a07/components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java
[modify] https://crrev.com/7b4b0d1feadff5ac989f0a73f190a35c08cc0a07/components/cronet/android/java/src/org/chromium/net/impl/NetworkExceptionImpl.java
[modify] https://crrev.com/7b4b0d1feadff5ac989f0a73f190a35c08cc0a07/components/cronet/android/url_request_error.cc
[modify] https://crrev.com/7b4b0d1feadff5ac989f0a73f190a35c08cc0a07/components/cronet/android/url_request_error.h

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

commit a12f4de94727def75758758e0cdebab6dc68275d
Author: Helen Li <xunjieli@chromium.org>
Date: Tue Aug 29 19:33:17 2017

[cronet] Add two more retryable error codes for BidirectionalStream

When encountering NetError.ERR_SPDY_PING_FAILED and ERR_QUIC_HANDSHAKE_FAILED,
return true for immediatelyRetryable() if BidirectionalStream is used.

This is done for BidirectionalStream and not for UrlRequest because UrlRequest
already retries on these errors in net::HttpNetworkTransaction.
net::BidirectionalStream sits below net::HttpNetworkTransaction so it
doesn't do automatic retries.

Bug:  757523 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: If9ea351ff444a3fb1cc4d35bdf05311f391d497e
Reviewed-on: https://chromium-review.googlesource.com/638434
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498200}
[modify] https://crrev.com/a12f4de94727def75758758e0cdebab6dc68275d/components/cronet/android/BUILD.gn
[add] https://crrev.com/a12f4de94727def75758758e0cdebab6dc68275d/components/cronet/android/java/src/org/chromium/net/impl/BidirectionalStreamNetworkException.java
[modify] https://crrev.com/a12f4de94727def75758758e0cdebab6dc68275d/components/cronet/android/java/src/org/chromium/net/impl/CronetBidirectionalStream.java
[modify] https://crrev.com/a12f4de94727def75758758e0cdebab6dc68275d/components/cronet/android/java/src/org/chromium/net/impl/NetworkExceptionImpl.java
[modify] https://crrev.com/a12f4de94727def75758758e0cdebab6dc68275d/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
[modify] https://crrev.com/a12f4de94727def75758758e0cdebab6dc68275d/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java

Ryan, is it okay to mark this as Fixed? As discussed earlier, BidirectionalStream doesn't do auto retries. If we want to support that, it needs an API change so Cronet knows how much to buffer internally.

Sure, that works for me!