Add an async method to QuicChromiumClientSession::Handle for

net/quic/chromium/quic_http_stream.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 <utility>
 
 #include "base/auto_reset.h"
 #include "base/callback_helpers.h"
@@ skipping 47 matching lines @@
       response_headers_received_(false),
       trailing_headers_received_(false),
       headers_bytes_received_(0),
       headers_bytes_sent_(0),
       closed_stream_received_bytes_(0),
       closed_stream_sent_bytes_(0),
       closed_is_first_stream_(false),
       user_buffer_len_(0),
       session_error_(ERR_UNEXPECTED),
       found_promise_(false),
-      push_handle_(nullptr),
       in_loop_(false),
       weak_factory_(this) {}
 
 QuicHttpStream::~QuicHttpStream() {
   CHECK(!in_loop_);
   Close(false);
 }
 
-bool QuicHttpStream::CheckVary(const SpdyHeaderBlock& client_request,
-                               const SpdyHeaderBlock& promise_request,
-                               const SpdyHeaderBlock& promise_response) {
-  HttpResponseInfo promise_response_info;
-
-  HttpRequestInfo promise_request_info;
-  ConvertHeaderBlockToHttpRequestHeaders(promise_request,
-                                         &promise_request_info.extra_headers);
-  HttpRequestInfo client_request_info;
-  ConvertHeaderBlockToHttpRequestHeaders(client_request,
-                                         &client_request_info.extra_headers);
-
-  if (!SpdyHeadersToHttpResponse(promise_response, &promise_response_info)) {
-    DLOG(WARNING) << "Invalid headers";
-    return false;
-  }
-
-  HttpVaryData vary_data;
-  if (!vary_data.Init(promise_request_info,
-                      *promise_response_info.headers.get())) {
-    // Promise didn't contain valid vary info, so URL match was sufficient.
-    return true;
-  }
-  // Now compare the client request for matching.
-  return vary_data.MatchesRequest(client_request_info,
-                                  *promise_response_info.headers.get());
-}
-
-void QuicHttpStream::OnRendezvousResult(QuicSpdyStream* stream) {
-  push_handle_ = nullptr;
-  if (stream) {
-    stream_ = static_cast<QuicChromiumClientStream*>(stream)->CreateHandle();
-  }
-
-  // callback_ should only be non-null in the case of asynchronous
-  // rendezvous; i.e. |Try()| returned QUIC_PENDING.
-  if (callback_.is_null())
-    return;
-
-  DCHECK_EQ(STATE_HANDLE_PROMISE_COMPLETE, next_state_);
-  if (!stream) {
-    // rendezvous has failed so proceed as with a non-push request.
-    next_state_ = STATE_REQUEST_STREAM;
-  }
-
-  OnIOComplete(OK);
-}
-
 HttpResponseInfo::ConnectionInfo QuicHttpStream::ConnectionInfoFromQuicVersion(
     QuicVersion quic_version) {
   switch (quic_version) {
     case QUIC_VERSION_UNSUPPORTED:
       return HttpResponseInfo::CONNECTION_INFO_QUIC_UNKNOWN_VERSION;
     case QUIC_VERSION_35:
       return HttpResponseInfo::CONNECTION_INFO_QUIC_35;
     case QUIC_VERSION_36:
       return HttpResponseInfo::CONNECTION_INFO_QUIC_36;
     case QUIC_VERSION_37:
@@ skipping 52 matching lines @@
 
   next_state_ = STATE_REQUEST_STREAM;
   int rv = DoLoop(OK);
   if (rv == ERR_IO_PENDING)
     callback_ = callback;
 
   return MapStreamError(rv);
 }
 
 int QuicHttpStream::DoHandlePromise() {
-  QuicAsyncStatus push_status = quic_session()->GetPushPromiseIndex()->Try(
-      request_headers_, this, &this->push_handle_);
+  int rv = quic_session()->RendezvousWithPromised(
+      request_headers_,
+      base::Bind(&QuicHttpStream::OnIOComplete, weak_factory_.GetWeakPtr()));

[xunjieli, 2017/06/21 20:55:28]

Should this be "base::Unretained(this)" given that QuicHttpStream owns the
handle?

[Ryan Hamilton, 2017/06/21 22:24:51]

Hm! Interesting. I think that's possible! But since all the other calls to the
handles base::Bind() with a WeakPtr, I'm inclined to leave this as-is in this CL
and then do a follow-up to switch over.

Though now that I think about it.. The HttpStream could pass an Unretained()
callback into the Handle. Could the Handle post a task to execute that callback,
which would end up running after the Handle is destroyed? I believe the Handle
does not actually do that, but I'm curious about the contract...

[xunjieli, 2017/06/22 14:43:30]

Acknowledged. Let's do it as a follow-up. If the Handle wants to take the
callback and post it as a task, it should wrap it in a base::Bind(). The handle
can use weak_ptr or base::Unretained() as appropriate for the wrapped callback.

 
-  switch (push_status) {
-    case QUIC_FAILURE:
-      // Push rendezvous failed.
-      next_state_ = STATE_REQUEST_STREAM;
-      break;
-    case QUIC_SUCCESS:
-      next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
-      break;
-    case QUIC_PENDING:
-      next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
-      return ERR_IO_PENDING;
+  if (rv == ERR_IO_PENDING) {
+    next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
+    return ERR_IO_PENDING;
   }
-  return OK;
+
+  if (rv < 0) {

[xunjieli, 2017/06/21 20:55:28]

nit: This branch is not needed. We can go directly to
STATE_HANDLE_PROMISE_COMPLETE, because DoHandlePromiseComplete() handles the
error case.

[Ryan Hamilton, 2017/06/21 22:24:51]

Hah! Clever! In fact, we don't need any of these branches.

Done.

+    // Push rendezvous failed.
+    next_state_ = STATE_REQUEST_STREAM;
+    return OK;
+  }
+
+  next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
+  return rv;
 }
 
 int QuicHttpStream::DoHandlePromiseComplete(int rv) {

[xunjieli, 2017/06/21 20:55:28]

optional nit: add a 
DCHECK_NE(ERR_IO_PENDING, rv)
DCHECK_LE(rv, OK)
here just to make the expected range of values for |rv| clearer.

[Ryan Hamilton, 2017/06/21 22:24:51]

Done.

-  if (rv != OK)
-    return rv;
+  if (rv != OK) {
+    // rendezvous has failed so proceed as with a non-push request.
+    next_state_ = STATE_REQUEST_STREAM;
+    return OK;
+  }
+
+  stream_ = quic_session()->ReleasePromisedStream();
 
   next_state_ = STATE_OPEN;
   stream_net_log_.AddEvent(
       NetLogEventType::QUIC_HTTP_STREAM_ADOPTED_PUSH_STREAM,
       base::Bind(&NetLogQuicPushStreamCallback, stream_->id(),
                  &request_info_->url));
   quic_session()->net_log().AddEvent(
       NetLogEventType::QUIC_HTTP_STREAM_ADOPTED_PUSH_STREAM,
       base::Bind(&NetLogQuicPushStreamCallback, stream_->id(),
                  &request_info_->url));
@@ skipping 276 matching lines @@
   base::AutoReset<bool> auto_reset_in_loop(&in_loop_, true);
   do {
     State state = next_state_;
     next_state_ = STATE_NONE;
     switch (state) {
       case STATE_HANDLE_PROMISE:
         CHECK_EQ(OK, rv);
         rv = DoHandlePromise();
         break;
       case STATE_HANDLE_PROMISE_COMPLETE:
-        CHECK_EQ(OK, rv);
         rv = DoHandlePromiseComplete(rv);
         break;
       case STATE_REQUEST_STREAM:
         CHECK_EQ(OK, rv);
         rv = DoRequestStream();
         break;
       case STATE_REQUEST_STREAM_COMPLETE:
         rv = DoRequestStreamComplete(rv);
         break;
       case STATE_SET_REQUEST_PRIORITY:
@@ skipping 209 matching lines @@
 int QuicHttpStream::HandleReadComplete(int rv) {
   if (stream_->IsDoneReading()) {
     stream_->OnFinRead();
     SetResponseStatus(OK);
     ResetStream();
   }
   return rv;
 }
 
 void QuicHttpStream::ResetStream() {
-  if (push_handle_) {
-    push_handle_->Cancel();
-    push_handle_ = nullptr;
-  }
-
   // If |request_body_stream_| is non-NULL, Reset it, to abort any in progress
   // read.
   if (request_body_stream_)
     request_body_stream_->Reset();
 
   if (!stream_)
     return;
 
   DCHECK_LE(stream_->NumBytesConsumed(), stream_->stream_bytes_read());
   // Only count the uniquely received bytes.
@@ skipping 46 matching lines @@
   // Explicit stream error are always fatal.
   if (stream_->stream_error() != QUIC_STREAM_NO_ERROR &&
       stream_->stream_error() != QUIC_STREAM_CONNECTION_ERROR) {
     return ERR_QUIC_PROTOCOL_ERROR;
   }
 
   return ERR_QUIC_PROTOCOL_ERROR;
 }
 
 }  // namespace net