Mark QUIC broken when the network blackholes after the handshake

net/quic/chromium/quic_http_stream_test.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/chromium/quic_http_stream.h"
 
 #include <stdint.h>
 
 #include <memory>
 #include <utility>
@@ skipping 85 matching lines @@
     QuicConnectionPeer::SetSendAlgorithm(this, send_algorithm);
   }
 };
 
 // Subclass of QuicHttpStream that closes itself when the first piece of data
 // is received.
 class AutoClosingStream : public QuicHttpStream {
  public:
   explicit AutoClosingStream(
       const base::WeakPtr<QuicChromiumClientSession>& session,
-      HttpServerProperties* http_server_properties)
-      : QuicHttpStream(session, http_server_properties) {}
+      HttpServerProperties* http_server_properties,
+      bool mark_quic_broken_when_network_suspected)
+      : QuicHttpStream(session,
+                       http_server_properties,
+                       mark_quic_broken_when_network_suspected) {}
 
   void OnHeadersAvailable(const SpdyHeaderBlock& headers,
                           size_t frame_len) override {
     Close(false);
   }
 
   void OnDataAvailable() override { Close(false); }
 };
 
 // UploadDataStream that always returns errors on data read.
@@ skipping 82 matching lines @@
     PacketToWrite(IoMode mode, int rv) : mode(mode), packet(nullptr), rv(rv) {}
     IoMode mode;
     QuicReceivedPacket* packet;
     int rv;
   };
 
   QuicHttpStreamTest()
       : use_closing_stream_(false),
         crypto_config_(crypto_test_utils::ProofVerifierForTesting()),
         read_buffer_(new IOBufferWithSize(4096)),
+        mark_quic_broken_when_network_suspected_(false),
         promise_id_(kServerDataStreamId1),
         stream_id_(kClientDataStreamId1),
         connection_id_(2),
         client_maker_(GetParam(),
                       connection_id_,
                       &clock_,
                       kDefaultServerHostName,
                       Perspective::IS_CLIENT),
         server_maker_(GetParam(),
                       connection_id_,
@@ skipping 105 matching lines @@
         QuicTime::Delta::FromMilliseconds(kQuicYieldAfterDurationMilliseconds),
         /*cert_verify_flags=*/0, DefaultQuicConfig(), &crypto_config_,
         "CONNECTION_UNKNOWN", dns_start, dns_end, &push_promise_index_, nullptr,
         base::ThreadTaskRunnerHandle::Get().get(),
         /*socket_performance_watcher=*/nullptr, net_log_.bound().net_log()));
     session_->Initialize();
     TestCompletionCallback callback;
     session_->CryptoConnect(callback.callback());
     EXPECT_TRUE(session_->IsCryptoHandshakeConfirmed());
     stream_.reset(use_closing_stream_
-                      ? new AutoClosingStream(session_->GetWeakPtr(),
-                                              &http_server_properties_)
-                      : new QuicHttpStream(session_->GetWeakPtr(),
-                                           &http_server_properties_));
+                      ? new AutoClosingStream(
+                            session_->GetWeakPtr(), &http_server_properties_,
+                            mark_quic_broken_when_network_suspected_)
+                      : new QuicHttpStream(
+                            session_->GetWeakPtr(), &http_server_properties_,
+                            mark_quic_broken_when_network_suspected_));
 
-    promised_stream_.reset(use_closing_stream_
-                               ? new AutoClosingStream(session_->GetWeakPtr(),
-                                                       &http_server_properties_)
-                               : new QuicHttpStream(session_->GetWeakPtr(),
-                                                    &http_server_properties_));
+    promised_stream_.reset(
+        use_closing_stream_
+            ? new AutoClosingStream(session_->GetWeakPtr(),
+                                    &http_server_properties_,
+                                    mark_quic_broken_when_network_suspected_)
+            : new QuicHttpStream(session_->GetWeakPtr(),
+                                 &http_server_properties_,
+                                 mark_quic_broken_when_network_suspected_));
 
     push_promise_[":path"] = "/bar";
     push_promise_[":authority"] = "www.example.org";
     push_promise_[":version"] = "HTTP/1.1";
     push_promise_[":method"] = "GET";
     push_promise_[":scheme"] = "https";
 
     promised_response_[":status"] = "200 OK";
     promised_response_[":version"] = "HTTP/1.1";
     promised_response_["content-type"] = "text/plain";
@@ skipping 222 matching lines @@
   TestCompletionCallback callback_;
   HttpRequestInfo request_;
   HttpRequestHeaders headers_;
   HttpResponseInfo response_;
   scoped_refptr<IOBufferWithSize> read_buffer_;
   SpdyHeaderBlock request_headers_;
   SpdyHeaderBlock response_headers_;
   string request_data_;
   string response_data_;
   QuicClientPushPromiseIndex push_promise_index_;
+  bool mark_quic_broken_when_network_suspected_;

[Bence, 2017/04/03 19:02:59]

This member is always false, so consider a file-scope const bool
kMarkQuicBrokenWhenNetworkSuspected to highlight this fact.

[Ryan Hamilton, 2017/04/05 19:26:20]

Done.

[Bence, 2017/04/05 23:49:46]

Sorry, I was rather thinking of setting kMarkQuicBrokenWhenNetworkSuspected
false and using it instead of its inverse everywhere. 
kMarkQuicBrokenWhenNetworkSuspected would then mean "whether to mark QUIC
broken" as opposed to "the value that would need to be used if QUIC was to be
marked broken" (as if it was an enum entry).

[Ryan Hamilton, 2017/04/08 00:07:23]

I see. As it happens, I ended up reverting the changes to this file.

 
   // For server push testing
   std::unique_ptr<QuicHttpStream> promised_stream_;
   SpdyHeaderBlock push_promise_;
   SpdyHeaderBlock promised_response_;
   const QuicStreamId promise_id_;
   string promise_url_;
   string serialized_push_promise_;
   const QuicStreamId stream_id_;
 
@@ skipping 121 matching lines @@
   request_.method = "GET";
   request_.url = GURL("http://www.example.org/");
   // Start first request.
   EXPECT_EQ(OK,
             stream_->InitializeStream(&request_, DEFAULT_PRIORITY,
                                       net_log_.bound(), callback_.callback()));
   EXPECT_EQ(OK,
             stream_->SendRequest(headers_, &response_, callback_.callback()));
 
   // Start a second request.
-  QuicHttpStream stream2(session_->GetWeakPtr(), &http_server_properties_);
+  QuicHttpStream stream2(session_->GetWeakPtr(), &http_server_properties_,
+                         mark_quic_broken_when_network_suspected_);
   TestCompletionCallback callback2;
   EXPECT_EQ(OK,
             stream2.InitializeStream(&request_, DEFAULT_PRIORITY,
                                      net_log_.bound(), callback2.callback()));
   EXPECT_EQ(OK,
             stream2.SendRequest(headers_, &response_, callback2.callback()));
 
   // Ack both requests.
   ProcessPacket(ConstructServerAckPacket(1, 0, 0));
 
@@ skipping 1430 matching lines @@
   EXPECT_TRUE(AtEof());
 
   // QuicHttpStream::GetTotalSent/ReceivedBytes includes only headers.
   EXPECT_EQ(static_cast<int64_t>(spdy_request_headers_frame_length),
             stream_->GetTotalSentBytes());
   EXPECT_EQ(0, stream_->GetTotalReceivedBytes());
 }
 
 }  // namespace test
 }  // namespace net