Change SPDY to call request_callback after data is sent.

net/spdy/spdy_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/spdy/spdy_http_stream.h"
 
 #include <algorithm>
 #include <list>
 #include <utility>
 
@@ skipping 284 matching lines @@
 void SpdyHttpStream::Cancel() {
   request_callback_.Reset();
   response_callback_.Reset();
   if (stream_.get()) {
     stream_->Cancel();
     DCHECK(!stream_.get());
   }
 }
 
 void SpdyHttpStream::OnRequestHeadersSent() {
-  if (!request_callback_.is_null())
-    DoRequestCallback(OK);
-
   // TODO(akalin): Do this immediately after sending the request

[Bence, 2016/06/13 13:53:36]

I do not exactly understand this TODO, but my impression is that it was either
already moot or you are resolving it with this change.  Therefore I suggest you
remove this TODO.

[maksims (do not use this acc), 2016/06/15 07:51:45]

Done.

   // headers.
-  if (HasUploadData())
+  if (HasUploadData()) {
     ReadAndSendRequestBodyData();
+  } else {
+    if (!request_callback_.is_null())
+      DoRequestCallback(OK);
+  }
 }
 
 SpdyResponseHeadersStatus SpdyHttpStream::OnResponseHeadersUpdated(
     const SpdyHeaderBlock& response_headers) {
   CHECK_EQ(response_headers_status_, RESPONSE_HEADERS_ARE_INCOMPLETE);
 
   if (!response_info_) {
     DCHECK_EQ(stream_->type(), SPDY_PUSH_STREAM);
     push_response_info_.reset(new HttpResponseInfo);
     response_info_ = push_response_info_.get();
@@ skipping 138 matching lines @@
   const bool eof = request_info_->upload_data_stream->IsEOF();
   // Only the final fame may have a length of 0.
   if (eof) {
     CHECK_GE(request_body_buf_size_, 0);
   } else {
     CHECK_GT(request_body_buf_size_, 0);
   }
   stream_->SendData(request_body_buf_.get(),
                     request_body_buf_size_,
                     eof ? NO_MORE_DATA_TO_SEND : MORE_DATA_TO_SEND);
+
+  if (!request_callback_.is_null())
+    DoRequestCallback(OK);

[mmenke, 2016/06/13 14:31:06]

BUG:  Shouldn't we only do this if eof is true?

[mmenke, 2016/06/13 14:48:27]

Actually, that's not enough.  ReadAndSendRequestBodyData should also do it, if
IsEOF returns true....  May be safest to just call this method directly with
net::OK, in that case.

We should also have a tests where we have an UploadDataStream, and we first
discover upload_data_stream->IsEOF returns true here and in the first call to
ReadAndSendRequestBodyData.  (I assume we already have the first test, but not
the second)

[maksims (do not use this acc), 2016/06/14 08:35:03]

Oh,oh, my mistake. I haven't noticed that. I checked the implementation of QUIC,
and it seems like it returns OK only after the whole operation is done.
What method????? Actually, when eof is met here, then we immideately face the
eof in ReadAndSendRequestBodyData().. Maybe because of the posttasks or drains.
But, anyway, It is always so and the req_cb is already null if I put it in the
ReadAndSendRequestBodyData() method.
The second? Which one? Didn't get it.

[mmenke, 2016/06/14 15:25:44]

This one....  If request_info_->upload_data_stream->IsEOF() returns true in
ReadAndSendRequestBodyData(), we could just call this one with a value of 0.
Test 1) Check that things work when we invoke DoRequestCallback(OK) here.  We
probably already have a such a test, but best to make sure (Can just put a CHECK
when we invoke the callback here and run tests, and see which one triggers it).
Test 2) Check that things work when we invoke DoRequestCallback(OK) in
SpdyHttpStream::ReadAndSendRequestBodyData.

[maksims (do not use this acc), 2016/06/15 07:51:45]

As I said if I call OnRequestBodyReadCompleted from the place you mentioned,
then the write fails. Thus, I just call the callback. I might have understand
you wrong.
Yes, more than a half of the tests call this Callback.

 }
 
 void SpdyHttpStream::ScheduleBufferedReadCallback() {
   // If there is already a scheduled DoBufferedReadCallback, don't issue
   // another one.  Mark that we have received more data and return.
   if (buffered_read_callback_pending_) {
     more_read_data_pending_ = true;
     return;
   }
 
@@ skipping 104 matching lines @@
   details->connection_info = HttpResponseInfo::CONNECTION_INFO_HTTP2;
   return;
 }
 
 void SpdyHttpStream::SetPriority(RequestPriority priority) {
   // TODO(akalin): Plumb this through to |stream_request_| and
   // |stream_|.
 }
 
 }  // namespace net