Mark QUIC broken when the network blackholes after the handshake

net/quic/chromium/quic_http_stream.cc

Index: net/quic/chromium/quic_http_stream.cc
diff --git a/net/quic/chromium/quic_http_stream.cc b/net/quic/chromium/quic_http_stream.cc
index 3b031b93f7ce6d1d9a74d68f82965c9f39d87734..ff5d095f2227086f5a1a3c8790e3742cf48f5edf 100644
--- a/net/quic/chromium/quic_http_stream.cc
+++ b/net/quic/chromium/quic_http_stream.cc
@@ -46,11 +46,15 @@ std::unique_ptr<base::Value> NetLogQuicPushStreamCallback(
 
 QuicHttpStream::QuicHttpStream(
     const base::WeakPtr<QuicChromiumClientSession>& session,
-    HttpServerProperties* http_server_properties)
+    HttpServerProperties* http_server_properties,
+    bool mark_quic_broken_when_network_suspected)
     : MultiplexedHttpStream(MultiplexedSessionHandle(session)),
       next_state_(STATE_NONE),
       session_(session),
+      server_id_(session->server_id()),
       http_server_properties_(http_server_properties),
+      mark_quic_broken_when_network_suspected_(
+          mark_quic_broken_when_network_suspected),
       quic_version_(session->GetQuicVersion()),
       session_error_(ERR_UNEXPECTED),
       was_handshake_confirmed_(session->IsCryptoHandshakeConfirmed()),
@@ -320,7 +324,6 @@ int QuicHttpStream::ReadResponseHeaders(const CompletionCallback& callback) {
 
   if (stream_ == nullptr)
     return GetResponseStatus();
-
   // Check if we already have the response headers. If so, return synchronously.
   if (response_headers_received_)
     return OK;
@@ -362,6 +365,7 @@ int QuicHttpStream::ReadResponseBody(IOBuffer* buf,
 }
 
 void QuicHttpStream::Close(bool /*not_reusable*/) {
+  session_error_ = ERR_ABORTED;

[Jana, 2017/04/06 02:29:18]

Why this change?

   SaveResponseStatus();
   // Note: the not_reusable flag has no meaning for QUIC streams.
   if (stream_) {
@@ -854,11 +858,18 @@ void QuicHttpStream::SetResponseStatus(int response_status) {
   response_status_ = response_status;
 }
 
+// Returns true if |error| is possibly caused by the network
+bool IsPossiblyNetworkError(QuicErrorCode error) {
+  return error == QUIC_NETWORK_IDLE_TIMEOUT ||
+         error == QUIC_TIMEOUTS_WITH_OPEN_STREAMS ||
+         error == QUIC_TOO_MANY_RTOS;

[Jana, 2017/04/06 02:29:18]

I'm surprised to see NETWORK_IDLE_TIMEOUT  treated as a network error. I also
don't think TOO_MANY_RTOS should cause QUIC to be marked as broken... this could
simply be a disconnected network, right?

[Ryan Hamilton, 2017/04/06 03:43:38]

The objective is to mark QUIC as broken when the network turns into a blackhole
after the handshake. Depending on how QUIC is configured, this may result in
TOO_MANY_RTOS (if the 5RTO experiment is enabled) or IDLE_NETWORK_TIMEOUT (if
that experiment is not active). IDLE_NETWORK_TIMEOUT is of course the normal way
for a session to be closed, but since this logic is running in the stream, that
means we have a timeout with an open stream, and hence we want to mark QUIC as
broken and fallback. 

Does that help?

[Jana, 2017/04/06 23:55:25]

Thanks for the explanation -- if you could add a comment to the effect that the
errors capture timeout with open streams, that would be useful. Something like
"SInce this stream is still open, a connection timeout is considered a network
error"

+}
+
 int QuicHttpStream::ComputeResponseStatus() const {
   DCHECK(!has_response_status_);
 
-  // If the handshake has failed this will be handled by the
-  // QuicStreamFactory and HttpStreamFactory to mark QUIC as broken.
+  // If the handshake has failed this will be handled by the QuicStreamFactory
+  // and HttpStreamFactory to mark QUIC as broken if TCP is actually working.
   if (!was_handshake_confirmed_)
     return ERR_QUIC_HANDSHAKE_FAILED;
 
@@ -872,6 +883,32 @@ int QuicHttpStream::ComputeResponseStatus() const {
   if (!response_info_)
     return ERR_CONNECTION_CLOSED;
 
+  // Explicit stream error are always fatal.

[Jana, 2017/04/06 02:29:18]

nit: error -> errors
I also don't understand this comment. What are streams errors fatal to? This
returns ERR_QUIC_PROTOCOL_ERROR, that's fatal to the request, since the error
causes the request to fail, doesn't it?

Ah, ok. I understand now -- that these errors are *always* fatal. I think this
flow is a bit confusing (to me)... I think it might be more readable if
IsPossiblyNetworkError encapsulated the logic to determine if this is something
that should be treated as a network error, and have a single branch here as
follows:
if (mark_quic_broken_when_network_suspected_ && 
    !IsPossiblyNetworkError(quic_connection_error_, quic_stream_error_)) {
    return ERR_QUIC_PROTOCOL_ERROR;
}
// Mark QUIC as broken ...

[Ryan Hamilton, 2017/04/06 03:43:38]

So it's a bit more complicated than that :/

There are 4 different ways that a stream gets closed:
1) The stream is explicitly closed by the caller
2) The session is closed by the QuicStreamFactory (say because of connection
migration or IP change)
3) The stream is reset (via RST_STREAM frame)
4) The connection is closed because of an error

The first two cases set the session_error_ member to the net error.
The third sets the quic_stream_error member to the QuicRstError code.
The fourth sets the quic_connection_error_ member to the QuicError code.

Only the fourth case results in broken, and then only when the
mark_quic_broken_when_network_suspected_ experiment is active. It's not clear to
me that moving the check of the stream error code up into IsPossiblyNetworkError
helps since those are never network caused, nor are the session_error_ which
would not be passed in. The only reason I created the IsPossiblyNetworkError()
method was to avoid enumerating them down at line 898 below, but perhaps it
would be cleaner to just remove the method and inline it there?

+  if (quic_stream_error_ != QUIC_STREAM_NO_ERROR &&
+      quic_stream_error_ != QUIC_STREAM_CONNECTION_ERROR) {
+    return ERR_QUIC_PROTOCOL_ERROR;
+  }
+
+  DCHECK_NE(QUIC_HANDSHAKE_TIMEOUT, quic_connection_error_);

[Jana, 2017/04/06 02:29:18]

What's the point of this DCHECK? The branch on line 873 should ensure this, no?

[Ryan Hamilton, 2017/04/06 03:43:38]

That's why it's a DCHECK :)

QUIC_HANDSHAKE_TIMEOUT seems like an error code that might be caused by the
network, and it IS a valid error code, but hopefully we never see it after the
handshake. Hence the DCHECK. (DCHECK are for verifying impossible conditions:

Use DCHECK() or NOTREACHED() as assertions, e.g. to document pre- and
post-conditions. A DCHECK() means “this condition must always be true”, not
“this condition is normally true, but perhaps not in exceptional cases.” Things
like disk corruption or strange network errors are examples of exceptional
circumstances that nevertheless should not result in DCHECK() failure.

[Jana, 2017/04/06 23:55:25]

Acknowledged.

+
+  if (!mark_quic_broken_when_network_suspected_)
+    return ERR_QUIC_PROTOCOL_ERROR;
+
+  // Error codes which could not have been caused by the network are fatal.

[Jana, 2017/04/06 02:29:18]

the use of "fatal" is confusing... perhaps you mean to say that only network
errors should result in marking QUIC  as broken, so requests can be retried?
Also, combine this branch with the one above (line 894).

[Ryan Hamilton, 2017/04/06 03:43:38]

By "fatal" I mean fatal to the request. Happy to hear rephrasing suggestions.

+  if (!IsPossiblyNetworkError(quic_connection_error_))
+    return ERR_QUIC_PROTOCOL_ERROR;
+
+  // For error codes which could have been generated by "the network", mark

[Jana, 2017/04/06 02:29:18]

odd to say that error codes could have been generated by the network, since the
network doesn't generate these codes... how about "errors caused by network
brokenness/issues"?

[Ryan Hamilton, 2017/04/06 03:43:38]

Done.

+  // QUIC as broken.
+  HistogramBrokenAlternateProtocolLocation(
+      BROKEN_ALTERNATE_PROTOCOL_LOCATION_QUIC_HTTP_STREAM);
+  http_server_properties_->MarkAlternativeServiceBroken(
+      AlternativeService(kProtoQUIC, server_id_.host(), server_id_.port()));
+  // If the headesr have not been received, return ERR_QUIC_BROKEN_ERROR to

[Jana, 2017/04/06 02:29:18]

nit: "headers"

[Ryan Hamilton, 2017/04/06 03:43:38]

Done.

[Ryan Hamilton, 2017/04/06 03:43:38]

Done.

+  // permit HttpNetworkTransaction to retry the request over TCP.
+  if (!response_headers_received_)
+    return ERR_QUIC_BROKEN_ERROR;
+
   return ERR_QUIC_PROTOCOL_ERROR;
 }