Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2064593002:
Change SPDY to call request_callback after data is sent. (Closed)
|
Created:
4 years, 6 months ago by maksims (do not use this acc) Modified:
4 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Ryan Hamilton Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange SPDY to call request_callback after data is sent.
This cl makes SPDY to match HTTP and QUIC implementations
and call the request_callback only after data is sent.
The unittests are corrected accordingly.
Committed: https://crrev.com/8d2df66d2b150b00ed046d809bd0a16b0a7b75b7
Cr-Commit-Position: refs/heads/master@{#400629}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 7
Patch Set 3 : call cb only after the whole data has been sent if there was data #
Total comments: 7
Patch Set 4 : unittest last empty chunk #
Total comments: 4
Patch Set 5 : append nullptr to data before request #
Total comments: 5
Patch Set 6 : mmenke comments #
Messages
Total messages: 28 (9 generated)
Description was changed from ========== Change SPDY to call request_callback after data is sent. This cl makes SPDY to match HTTP and QUIC implementations and call the request_callback only after data is sent. The unittests are corrected accordingly. ========== to ========== Change SPDY to call request_callback after data is sent. This cl makes SPDY to match HTTP and QUIC implementations and call the request_callback only after data is sent. The unittests are corrected accordingly. ==========
maksim.sisov@intel.com changed reviewers: + bnc@chromium.org, mmenke@chromium.org, rch@chromium.org
maksim.sisov@intel.com changed reviewers: - rch@chromium.org
please review
LGTM with optional nit. Thank you. https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:305: // TODO(akalin): Do this immediately after sending the request 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.
https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:473: DoRequestCallback(OK); BUG: Shouldn't we only do this if eof is true?
https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:473: DoRequestCallback(OK); On 2016/06/13 14:31:06, mmenke wrote: > BUG: Shouldn't we only do this if eof is true? 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)
https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:473: DoRequestCallback(OK); On 2016/06/13 14:48:27, mmenke wrote: > On 2016/06/13 14:31:06, mmenke wrote: > > BUG: Shouldn't we only do this if eof is true? > 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. > 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. > 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. > 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) The second? Which one? Didn't get it.
https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:473: DoRequestCallback(OK); On 2016/06/14 08:35:03, maksims wrote: > On 2016/06/13 14:48:27, mmenke wrote: > > On 2016/06/13 14:31:06, mmenke wrote: > > > BUG: Shouldn't we only do this if eof is true? > > > 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. > > > 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. > > > 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. This one.... If request_info_->upload_data_stream->IsEOF() returns true in ReadAndSendRequestBodyData(), we could just call this one with a value of 0. > > 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) > > The second? Which one? Didn't get it. 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. https://codereview.chromium.org/2064593002/diff/40001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:441: // paranoid. I don't believe this is true - when uploading chunked files, we may not learn it's a 0-byte file until the call here to upload_data_stream->IsEOF() returns true. That's the reason the IsEOF check is needed here in the first place. https://codereview.chromium.org/2064593002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:444: } nit: Remove braces https://codereview.chromium.org/2064593002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:469: // Call the cb only after the whole data is sent. nit: "after the whole file is sent." or "after all data is sent." The reason for that is that "whole" means all of a single thing, but "data" is plural. https://codereview.chromium.org/2064593002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:472: } nit: Remove braces
On 2016/06/14 15:25:44, mmenke wrote: > https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... > File net/spdy/spdy_http_stream.cc (right): > > https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... > net/spdy/spdy_http_stream.cc:473: DoRequestCallback(OK); > On 2016/06/14 08:35:03, maksims wrote: > > On 2016/06/13 14:48:27, mmenke wrote: > > > On 2016/06/13 14:31:06, mmenke wrote: > > > > BUG: Shouldn't we only do this if eof is true? > > > > > 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. > > > > > 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. > > > > > 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. > > This one.... If request_info_->upload_data_stream->IsEOF() returns true in > ReadAndSendRequestBodyData(), we could just call this one with a value of 0. I tried that...Then the write() method fails and test crashes.
On 2016/06/14 17:24:15, maksims wrote: > On 2016/06/14 15:25:44, mmenke wrote: > > > https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... > > File net/spdy/spdy_http_stream.cc (right): > > > > > https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... > > net/spdy/spdy_http_stream.cc:473: DoRequestCallback(OK); > > On 2016/06/14 08:35:03, maksims wrote: > > > On 2016/06/13 14:48:27, mmenke wrote: > > > > On 2016/06/13 14:31:06, mmenke wrote: > > > > > BUG: Shouldn't we only do this if eof is true? > > > > > > > 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. > > > > > > > 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. > > > > > > > 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. > > > > This one.... If request_info_->upload_data_stream->IsEOF() returns true in > > ReadAndSendRequestBodyData(), we could just call this one with a value of 0. > I tried that...Then the write() method fails and test crashes. SendData**
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:305: // TODO(akalin): Do this immediately after sending the request On 2016/06/13 13:53:36, Bence OOO until June 20 wrote: > 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. Done. https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:473: DoRequestCallback(OK); On 2016/06/14 15:25:44, mmenke wrote: > On 2016/06/14 08:35:03, maksims wrote: > > On 2016/06/13 14:48:27, mmenke wrote: > > > On 2016/06/13 14:31:06, mmenke wrote: > > > > BUG: Shouldn't we only do this if eof is true? > > > > > 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. > > > > > 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. > > > > > 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. > > This one.... If request_info_->upload_data_stream->IsEOF() returns true in > ReadAndSendRequestBodyData(), we could just call this one with a value of 0. 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. > > > > 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) > > > > The second? Which one? Didn't get it. > > 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. Yes, more than a half of the tests call this Callback. https://codereview.chromium.org/2064593002/diff/40001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:441: // paranoid. On 2016/06/14 15:25:44, mmenke wrote: > I don't believe this is true - when uploading chunked files, we may not learn > it's a 0-byte file until the call here to upload_data_stream->IsEOF() returns > true. > > That's the reason the IsEOF check is needed here in the first place. Check the SendChunkedPostLastEmpty that I added. As you have said there is a 0-byte chunk, but the eof is checked first in OnRequestBodyReadCompleted() and then we check the eof here due to the DoLoop in SpdySession and then we exit. https://codereview.chromium.org/2064593002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:469: // Call the cb only after the whole data is sent. On 2016/06/14 15:25:44, mmenke wrote: > nit: "after the whole file is sent." or "after all data is sent." > > The reason for that is that "whole" means all of a single thing, but "data" is > plural. Done. https://codereview.chromium.org/2064593002/diff/40001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:472: } On 2016/06/14 15:25:44, mmenke wrote: > nit: Remove braces Done.
https://codereview.chromium.org/2064593002/diff/100001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/100001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream.cc:441: // Never called!!! What if we have a chunked upload with no data? - i.e., we just call upload_stream.AppendData(nullptr, 0, true);, before we start the request?
On 2016/06/13 14:31:07, mmenke wrote: > https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... > File net/spdy/spdy_http_stream.cc (right): > > https://codereview.chromium.org/2064593002/diff/20001/net/spdy/spdy_http_stre... > net/spdy/spdy_http_stream.cc:473: DoRequestCallback(OK); > BUG: Shouldn't we only do this if eof is true? I'm very sorry about this oversight. mmenke, thank you for taking over this in my absence.
https://codereview.chromium.org/2064593002/diff/100001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/100001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream.cc:441: // Never called!!! On 2016/06/15 16:15:18, mmenke wrote: > What if we have a chunked upload with no data? - i.e., we just call > upload_stream.AppendData(nullptr, 0, true);, before we start the request? Nope, check the SendChunkedPostLastEmpty unit test. First, the eof is hit on the line 467 and then here, but the cb is empty by this time.
https://codereview.chromium.org/2064593002/diff/100001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/100001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream.cc:441: // Never called!!! On 2016/06/16 05:12:46, maksims wrote: > On 2016/06/15 16:15:18, mmenke wrote: > > What if we have a chunked upload with no data? - i.e., we just call > > upload_stream.AppendData(nullptr, 0, true);, before we start the request? > > Nope, check the SendChunkedPostLastEmpty unit test. > > First, the eof is hit on the line 467 and then here, but the cb is empty by this > time. And we know about the eof only after upload_data_stream->Read() has been called. Thus, we cannot know if there is eof before the actual read.
https://codereview.chromium.org/2064593002/diff/100001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/100001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream.cc:441: // Never called!!! And I guess the only way to envoke the first eof is by implementing own reader that sets the UploadDataStream's bool is_chunked_ to false; Then is_eof_ will be true on OnInitCompleted. And the problem with the .AppendData(nullptr, 0, true); is that the ReadChunk will not be envoked, because there is no buffer. The method just returns without setting the eof to true. And when the SendRequest is envoked, buffer is created and passed to the UploadDataStream, which reads the data and then sets eof to true. That is why the eof is true on line 467 and then here.
Any suggestions?
On 2016/06/16 06:52:28, maksims wrote: > https://codereview.chromium.org/2064593002/diff/100001/net/spdy/spdy_http_str... > File net/spdy/spdy_http_stream.cc (right): > > https://codereview.chromium.org/2064593002/diff/100001/net/spdy/spdy_http_str... > net/spdy/spdy_http_stream.cc:441: // Never called!!! > And I guess the only way to envoke the first eof is by implementing own reader > that sets the UploadDataStream's bool is_chunked_ to false; > > Then is_eof_ will be true on OnInitCompleted. > > And the problem with the .AppendData(nullptr, 0, true); is that the ReadChunk > will not be envoked, because there is no buffer. The method just returns without > setting the eof to true. And when the SendRequest is envoked, buffer is created > and passed to the UploadDataStream, which reads the data and then sets eof to > true. That is why the eof is true on line 467 and then here. I'm sorry, forgot this one was on my plate (When I checked "your SPDY CL" to determine if it was on my plate, I saw the other CL and missed this one). So I think we actually need to create a 0-byte non-chunked response to get there...Except then HasUploadData returns 0, so I don't actually think that case *can* be reached, but it's ambiguous enough that I think we should keep the code for handling it, we just can't test it. LGTM, after comments are addressed. https://codereview.chromium.org/2064593002/diff/120001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2064593002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream.cc:438: if (request_info_->upload_data_stream->IsEOF()) { Maybe add a comment that this case never currently happens, but seems best to handle, out of paranoia? We generally shouldn't handle things that we don't expect to happen, but this is just close enough to possible that I think we should handle it here. https://codereview.chromium.org/2064593002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream.cc:442: DCHECK(false); Remove this DCHECK and the comment (And remove braces). https://codereview.chromium.org/2064593002/diff/120001/net/spdy/spdy_http_str... File net/spdy/spdy_http_stream_unittest.cc (right): https://codereview.chromium.org/2064593002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:402: // CreateMockWrite(*chunk3, 3), Remove commented out code. https://codereview.chromium.org/2064593002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:440: base::RunLoop().RunUntilIdle(); This isn't needed. https://codereview.chromium.org/2064593002/diff/120001/net/spdy/spdy_http_str... net/spdy/spdy_http_stream_unittest.cc:449: // in the pool anymore. remove "we"
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2064593002/#ps140001 (title: "mmenke comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2064593002/140001
Message was sent while issue was closed.
Description was changed from ========== Change SPDY to call request_callback after data is sent. This cl makes SPDY to match HTTP and QUIC implementations and call the request_callback only after data is sent. The unittests are corrected accordingly. ========== to ========== Change SPDY to call request_callback after data is sent. This cl makes SPDY to match HTTP and QUIC implementations and call the request_callback only after data is sent. The unittests are corrected accordingly. ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Change SPDY to call request_callback after data is sent. This cl makes SPDY to match HTTP and QUIC implementations and call the request_callback only after data is sent. The unittests are corrected accordingly. ========== to ========== Change SPDY to call request_callback after data is sent. This cl makes SPDY to match HTTP and QUIC implementations and call the request_callback only after data is sent. The unittests are corrected accordingly. Committed: https://crrev.com/8d2df66d2b150b00ed046d809bd0a16b0a7b75b7 Cr-Commit-Position: refs/heads/master@{#400629} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8d2df66d2b150b00ed046d809bd0a16b0a7b75b7 Cr-Commit-Position: refs/heads/master@{#400629} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
