Add an async ReadInitialHeaders method to QuicChromiumClientStream::Handle

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 89 matching lines @@
 
 // Subclass of QuicHttpStream that closes itself when the first piece of data
 // is received.
 class AutoClosingStream : public QuicHttpStream {
  public:
   explicit AutoClosingStream(
       std::unique_ptr<QuicChromiumClientSession::Handle> session,
       HttpServerProperties* http_server_properties)
       : QuicHttpStream(std::move(session), http_server_properties) {}
 
-  void OnInitialHeadersAvailable(const SpdyHeaderBlock& headers,
-                                 size_t frame_len) override {
-    Close(false);
-  }
-
   void OnTrailingHeadersAvailable(const SpdyHeaderBlock& headers,
                                   size_t frame_len) override {
     Close(false);
   }
 
   void OnDataAvailable() override { Close(false); }
 };
 
 // UploadDataStream that always returns errors on data read.
 class ReadErrorUploadDataStream : public UploadDataStream {
@@ skipping 53 matching lines @@
 
 class QuicHttpStreamPeer {
  public:
   static QuicChromiumClientStream::Handle* GetQuicChromiumClientStream(
       QuicHttpStream* stream) {
     return stream->stream_.get();
   }
 };
 
 class QuicHttpStreamTest : public ::testing::TestWithParam<QuicVersion> {
+ public:
+  void CloseStream(QuicHttpStream* stream, int /*rv*/) { stream->Close(false); }
+
  protected:
   static const bool kFin = true;
   static const bool kIncludeVersion = true;
   static const bool kIncludeCongestionFeedback = true;
 
   // Holds a packet to be written to the wire, and the IO mode that should
   // be used by the mock socket when performing the write.
   struct PacketToWrite {
     PacketToWrite(IoMode mode, QuicReceivedPacket* packet)
         : mode(mode), packet(packet) {}
@@ skipping 954 matching lines @@
 
   // Ack both packets in the request.
   ProcessPacket(ConstructServerAckPacket(1, 0, 0, 0));
 
   // Send the response headers (but not the body).
   SetResponse("200 OK", string());
   size_t spdy_response_headers_frame_length;
   ProcessPacket(ConstructResponseHeadersPacket(
       2, !kFin, &spdy_response_headers_frame_length));
 
-  // The headers have arrived, but they are delivered asynchronously.
-  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()),
-              IsError(ERR_IO_PENDING));
-  EXPECT_THAT(callback_.WaitForResult(), IsOk());
+  // The headers have already arrived.
+  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()), IsOk());
   ASSERT_TRUE(response_.headers.get());
   EXPECT_EQ(200, response_.headers->response_code());
   EXPECT_TRUE(response_.headers->HasHeaderValue("Content-Type", "text/plain"));
 
   // Send the response body.
   const char kResponseBody[] = "Hello world!";
   ProcessPacket(ConstructServerDataPacket(3, false, kFin, 0, kResponseBody));
   // Since the body has already arrived, this should return immediately.
   EXPECT_EQ(static_cast<int>(strlen(kResponseBody)),
             stream_->ReadResponseBody(read_buffer_.get(), read_buffer_->size(),
@@ skipping 49 matching lines @@
 
   // Ack both packets in the request.
   ProcessPacket(ConstructServerAckPacket(1, 0, 0, 0));
 
   // Send the response headers (but not the body).
   SetResponse("200 OK", string());
   size_t spdy_response_headers_frame_length;
   ProcessPacket(ConstructResponseHeadersPacket(
       2, !kFin, &spdy_response_headers_frame_length));
 
-  // The headers have arrived, but they are delivered asynchronously
-  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()),
-              IsError(ERR_IO_PENDING));
-  EXPECT_THAT(callback_.WaitForResult(), IsOk());
+  // The headers have already arrived.
+  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()), IsOk());
   ASSERT_TRUE(response_.headers.get());
   EXPECT_EQ(200, response_.headers->response_code());
   EXPECT_TRUE(response_.headers->HasHeaderValue("Content-Type", "text/plain"));
 
   // Send the response body.
   const char kResponseBody[] = "Hello world!";
   ProcessPacket(ConstructServerDataPacket(
       3, false, kFin, response_data_.length(), kResponseBody));
 
   // Since the body has already arrived, this should return immediately.
@@ skipping 49 matching lines @@
   EXPECT_THAT(callback_.WaitForResult(), IsOk());
 
   ProcessPacket(ConstructServerAckPacket(1, 0, 0, 0));
 
   // Send the response headers (but not the body).
   SetResponse("200 OK", string());
   size_t spdy_response_headers_frame_length;
   ProcessPacket(ConstructResponseHeadersPacket(
       2, !kFin, &spdy_response_headers_frame_length));
 
-  // The headers have arrived, but they are delivered asynchronously
-  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()),
-              IsError(ERR_IO_PENDING));
-  EXPECT_THAT(callback_.WaitForResult(), IsOk());
+  // The headers have already arrived.
+  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()), IsOk());
   ASSERT_TRUE(response_.headers.get());
   EXPECT_EQ(200, response_.headers->response_code());
   EXPECT_TRUE(response_.headers->HasHeaderValue("Content-Type", "text/plain"));
 
   // Send the response body.
   const char kResponseBody[] = "Hello world!";
   ProcessPacket(ConstructServerDataPacket(
       3, false, kFin, response_data_.length(), kResponseBody));
 
   // The body has arrived, but it is delivered asynchronously
@@ skipping 44 matching lines @@
   EXPECT_THAT(callback_.WaitForResult(), IsOk());
 
   ProcessPacket(ConstructServerAckPacket(1, 0, 0, 0));
 
   // Send the response headers (but not the body).
   SetResponse("200 OK", string());
   size_t spdy_response_headers_frame_length;
   ProcessPacket(ConstructResponseHeadersPacket(
       2, !kFin, &spdy_response_headers_frame_length));
 
-  // The headers have arrived, but they are delivered asynchronously
-  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()),
-              IsError(ERR_IO_PENDING));
-  EXPECT_THAT(callback_.WaitForResult(), IsOk());
+  // The headers have already arrived.
+  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()), IsOk());
   ASSERT_TRUE(response_.headers.get());
   EXPECT_EQ(200, response_.headers->response_code());
   EXPECT_TRUE(response_.headers->HasHeaderValue("Content-Type", "text/plain"));
 
   // Send the response body.
   const char kResponseBody[] = "Hello world!";
   ProcessPacket(ConstructServerDataPacket(
       3, false, kFin, response_data_.length(), kResponseBody));
 
   // The body has arrived, but it is delivered asynchronously
@@ skipping 30 matching lines @@
   request_.url = GURL("https://www.example.org/");
 
   EXPECT_EQ(OK,
             stream_->InitializeStream(&request_, DEFAULT_PRIORITY,
                                       net_log_.bound(), callback_.callback()));
   EXPECT_EQ(OK,
             stream_->SendRequest(headers_, &response_, callback_.callback()));
 
   // Ack the request.
   ProcessPacket(ConstructServerAckPacket(1, 0, 0, 0));
-  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()),
+  EXPECT_THAT(stream_->ReadResponseHeaders(
+                  base::Bind(&QuicHttpStreamTest::CloseStream,
+                             base::Unretained(this), stream_.get())),
               IsError(ERR_IO_PENDING));
 
   // Send the response with a body.
   SetResponse("404 OK", "hello world!");
   // In the course of processing this packet, the QuicHttpStream close itself.

[xunjieli, 2017/05/10 18:58:08]

This test is no longer testing what it claims to do. 
The response header packet will be processed by QuicHttpStream, right? Could you
rework this test so that the QuicHttpStream is destroyed before response header
packet?

One suggestion: 
When ReadResponseHeaders returns ERR_IO_PENDING, invoke CloseStream() right away
rather than doing CloseStream() as a completion callback.

[Ryan Hamilton, 2017/05/10 19:33:46]

The test says that the QuicHttpStream will be close itself while the headers
packet is being processed, and I think that's what the test doest. It start s a
read before the packet arrives. Then when the packet arrives, the headers are
handed to the stream, and the stream invokes the OnInitialHeaders method of the
handle, which in turn invokes the read callback on the QuicHttpStream which
closes the stream. I think that matches up.

(ProcessPacket hand the packet to the QuicConnection, not the QuicHttpStream)

[xunjieli, 2017/05/10 19:46:58]

Acknowledged. The comment that confused me is line 1438. The previous test is
checking that the QuicHttpStream processed 0 header bytes. Could you update the
comment?

[Ryan Hamilton, 2017/05/10 19:57:55]

Ah good point. Yeah, that's confusing. Done.

   ProcessPacket(ConstructResponseHeadersPacket(2, kFin, nullptr));
 
   base::RunLoop().RunUntilIdle();
 
   EXPECT_TRUE(AtEof());
 
   // QuicHttpStream::GetTotalSent/ReceivedBytes currently only includes the
   // headers and payload.
   EXPECT_EQ(static_cast<int64_t>(spdy_request_headers_frame_length),
             stream_->GetTotalSentBytes());
   // Zero since the stream is closed before processing the headers.
-  EXPECT_EQ(0, stream_->GetTotalReceivedBytes());
+  EXPECT_EQ(31, stream_->GetTotalReceivedBytes());
 }
 
 TEST_P(QuicHttpStreamTest, Priority) {
   SetRequest("GET", "/", MEDIUM);
   size_t spdy_request_headers_frame_length;
   QuicStreamOffset header_stream_offset = 0;
   AddWrite(ConstructInitialSettingsPacket(&header_stream_offset));
   AddWrite(InnerConstructRequestHeadersPacket(
       2, GetNthClientInitiatedStreamId(0), kIncludeVersion, kFin, MEDIUM,
       &spdy_request_headers_frame_length, &header_stream_offset));
@@ skipping 15 matching lines @@
 
   EXPECT_EQ(OK,
             stream_->SendRequest(headers_, &response_, callback_.callback()));
 
   // Check that priority has now dropped back to MEDIUM.
   DCHECK_EQ(MEDIUM,
             ConvertQuicPriorityToRequestPriority(reliable_stream->priority()));
 
   // Ack the request.
   ProcessPacket(ConstructServerAckPacket(1, 0, 0, 0));
-  EXPECT_THAT(stream_->ReadResponseHeaders(callback_.callback()),
+  EXPECT_THAT(stream_->ReadResponseHeaders(
+                  base::Bind(&QuicHttpStreamTest::CloseStream,

[xunjieli, 2017/05/10 18:58:08]

This test doesn't need the CloseStream/"Close early" logic. We could use
|callback_.callback()| and wait for callback_ to complete.

[Ryan Hamilton, 2017/05/10 19:33:46]

Actually the test does rely on the stream closing currently because of the 3rd
write. But that doesn't seem important to the test, so I'll nuke it. Done.

+                             base::Unretained(this), stream_.get())),
               IsError(ERR_IO_PENDING));
 
   // Send the response with a body.
   SetResponse("404 OK", "hello world!");
   // In the course of processing this packet, the QuicHttpStream close itself.
   ProcessPacket(ConstructResponseHeadersPacket(2, kFin, nullptr));
 
   base::RunLoop().RunUntilIdle();
 
   EXPECT_TRUE(AtEof());
 
   // QuicHttpStream::GetTotalSent/ReceivedBytes currently only includes the
   // headers and payload.
   EXPECT_EQ(static_cast<int64_t>(spdy_request_headers_frame_length),
             stream_->GetTotalSentBytes());
   // Zero since the stream is closed before processing the headers.

[xunjieli, 2017/05/10 18:58:08]

The comment is no longer accurate. Could you update this line?

[Ryan Hamilton, 2017/05/10 19:33:46]

Done.

-  EXPECT_EQ(0, stream_->GetTotalReceivedBytes());
+  EXPECT_EQ(31, stream_->GetTotalReceivedBytes());

[xunjieli, 2017/05/10 18:58:08]

Can we get "31" from ConstructResponseHeadersPacket() ?

[Ryan Hamilton, 2017/05/10 19:33:46]

Done.

 }
 
 // Regression test for http://crbug.com/294870
 TEST_P(QuicHttpStreamTest, CheckPriorityWithNoDelegate) {
   SetRequest("GET", "/", MEDIUM);
   use_closing_stream_ = true;
   QuicStreamOffset header_stream_offset = 0;
   AddWrite(ConstructInitialSettingsPacket(&header_stream_offset));
   AddWrite(ConstructClientRstStreamPacket(2));
 
@@ skipping 660 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