Cancel a promised stream if it is for a request with a body in QuicHttpStream.

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 16447899be3ce42ac65ddede9bcb790ee07c306e..446043104d49a878630e702e7d8937e85acbbadf 100644
--- a/net/quic/chromium/quic_http_stream.cc
+++ b/net/quic/chromium/quic_http_stream.cc
@@ -117,9 +117,8 @@ void QuicHttpStream::OnRendezvousResult(QuicSpdyStream* stream) {
   if (callback_.is_null())
     return;
 
-  if (stream) {
-    next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
-  } else {
+  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;
   }
@@ -174,16 +173,6 @@ int QuicHttpStream::InitializeStream(const HttpRequestInfo* request_info,
   return rv;
 }
 
-bool QuicHttpStream::CancelPromiseIfHasBody() {
-  if (!request_body_stream_)
-    return false;
-
-  // A request with a body is ineligble for push.
-  this->push_handle_->Cancel();
-  this->push_handle_ = nullptr;
-  return true;
-}
-
 int QuicHttpStream::DoHandlePromise() {
   QuicAsyncStatus push_status = session_->push_promise_index()->Try(
       request_headers_, this, &this->push_handle_);
@@ -197,14 +186,8 @@ int QuicHttpStream::DoHandlePromise() {
       next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
       break;
     case QUIC_PENDING:
-      if (!CancelPromiseIfHasBody()) {
-        next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
-        // Have a promise but the promised stream doesn't exist yet.
-        // Still have to do validation before accepting the promised
-        // stream for sure.
-        return ERR_IO_PENDING;
-      }
-      next_state_ = STATE_REQUEST_STREAM;
+      next_state_ = STATE_HANDLE_PROMISE_COMPLETE;
+      return ERR_IO_PENDING;
   }
   return OK;
 }
@@ -213,11 +196,6 @@ int QuicHttpStream::DoHandlePromiseComplete(int rv) {
   if (rv != OK)
     return rv;
 
-  if (CancelPromiseIfHasBody()) {
-    next_state_ = STATE_REQUEST_STREAM;
-    return OK;
-  }
-
   next_state_ = STATE_OPEN;
   stream_net_log_.AddEvent(
       NetLogEventType::QUIC_HTTP_STREAM_ADOPTED_PUSH_STREAM,
@@ -259,6 +237,18 @@ int QuicHttpStream::SendRequest(const HttpRequestHeaders& request_headers,
   // Store the request body.
   request_body_stream_ = request_info_->upload_data_stream;
   if (request_body_stream_) {
+    // A request with a body is ineligble for push, so reset the

[Buck, 2016/09/15 17:13:58]

nit: s/ineligble/ineligible/

[Ryan Hamilton, 2016/09/15 19:09:06]

Done.

+    // promised stream and request a new stream.
+    if (found_promise_) {
+      found_promise_ = false;
+      std::string url(request_info_->url.spec());
+      QuicClientPromisedInfo* promised =
+          session_->push_promise_index()->GetPromised(url);
+      if (promised != nullptr) {
+        session_->ResetPromised(promised->id(), QUIC_STREAM_CANCELLED);
+      }
+    }
+

[Buck, 2016/09/15 17:13:58]

totally up to you, but I feel like this would be nicer if it just did
push_handle_->Cancel()

Of course, you don't have push_handle_ yet, but maybe something like this
refactor:

  o rename TryHandle to PushHandle
      o in addition to Cancel(), it also has Try()
Refactor the current QuicClientPushPromiseIndex::Try() so into two bits:
  o new method QuicClientPushPromiseIndex::Lookup() (returns PushHandle)
           o using this, found_promised_ could be consolidated into push_handle_
  o remainder of Try() logic goes into new PushHandle::Try() method

...or I could do all that in a followup CL.

[Ryan Hamilton, 2016/09/15 19:09:06]

Agreed! I was actually expecting to be able to do that. But as you observe
below, we don't yet have the handle.
Sounds like a nice refactor. If you'd like to do it, that sounds great to me!

     // TODO(rch): Can we be more precise about when to allocate
     // raw_request_body_buf_. Removed the following check. DoReadRequestBody()
     // was being called even if we didn't yet allocate raw_request_body_buf_.
@@ -278,9 +268,6 @@ int QuicHttpStream::SendRequest(const HttpRequestHeaders& request_headers,
   int rv;
 
   if (found_promise_) {
-    // TODO(rch): If this request has a body, instead of waiting for the pushed
-    // headers to arrive before canceling the push we could cancel the pushed
-    // stream now and go straight to STATE_REQUEST_STREAM.
     next_state_ = STATE_HANDLE_PROMISE;
   } else {
     next_state_ = STATE_SET_REQUEST_PRIORITY;