QuicChromiumClientStream reports 0 byte read before trailers notification is sent |
|||||||||||
Issue description
If trailers are received after a QuicChromiumClientStream::Read() returned ERR_IO_PENDING, QuicChromiumClientStream::Delegate::OnDataAvailable() will inform the Delegate of the 0 byte read before Delegate::OnHeadersReceived(trailers) is invoked.
Elsewhere in net/, 0 byte read indicates EOF and client can assume there's no more data after that. However, since trailers notification is sent after 0 byte read, this led to incorrect ordering of events (See internal bug 31930496).
This behavior is captured in the following unit test where OnDataAvailable() is invoked before OnHeadersAvailable(trailers) when |stream_| received trailers.
TEST_P(QuicChromiumClientStreamTest, MarkTrailersConsumedWhenNotifyDelegate) {
.....
stream_->OnStreamHeaders(uncompressed_trailers);
stream_->OnStreamHeadersComplete(true, uncompressed_trailers.length());
EXPECT_FALSE(stream_->IsDoneReading());
// Now the pending should complete. Make sure that IsDoneReading() is false
// even though ReadData returns 0 byte, because OnHeadersAvailable callback
// comes after this OnDataAvailable callback.
base::RunLoop run_loop2;
EXPECT_CALL(delegate_, OnDataAvailable())
.Times(1)
.WillOnce(testing::DoAll(
testing::Invoke(CreateFunctor(&QuicChromiumClientStreamTest::ReadData,
base::Unretained(this), StringPiece())),
testing::InvokeWithoutArgs([&run_loop2]() { run_loop2.Quit(); })));
run_loop2.Run();
// Make sure that the stream is not closed, even though ReadData returns 0.
EXPECT_FALSE(stream_->IsDoneReading());
// The OnHeadersAvailable call should follow.
base::RunLoop run_loop3;
EXPECT_CALL(delegate_,
OnHeadersAvailableMock(_, uncompressed_trailers.length()))
.WillOnce(
testing::InvokeWithoutArgs([&run_loop3]() { run_loop3.Quit(); }));
run_loop3.Run();
// Make sure the stream is properly closed since trailers and data are all
// consumed.
EXPECT_TRUE(stream_->IsDoneReading());
// Make sure kFinalOffsetHeaderKey is gone from the delivered actual trailers.
trailers.erase(kFinalOffsetHeaderKey);
EXPECT_EQ(trailers, delegate_.headers_);
base::RunLoop().RunUntilIdle();
EXPECT_CALL(delegate_, OnClose());
}
,
Oct 5 2016
I haven't thought about that. Let me try patching this in and see if it works. Thanks for the suggestion!
,
Oct 5 2016
,
Oct 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7640faa369a7769b97719938c70e4e4910f476c3 commit 7640faa369a7769b97719938c70e4e4910f476c3 Author: xunjieli <xunjieli@chromium.org> Date: Thu Oct 06 00:45:42 2016 Remove OnDataAvailable() notification when trailers are to be delivered This CL used the change that is suggested by mpw@ in https://bugs.chromium.org/p/chromium/issues/detail?id=652935#c1. Specifically, this CL removes QuicChromiumClientStream::Delegate::OnDataAvailable() notification if there is no byte can be read and trailers are to be delivered. This CL adjusted unit tests to test the correct ordering of events. BUG= 652935 Review-Url: https://codereview.chromium.org/2393233002 Cr-Commit-Position: refs/heads/master@{#423357} [modify] https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3/net/quic/chromium/bidirectional_stream_quic_impl.cc [modify] https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc [modify] https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3/net/quic/chromium/quic_chromium_client_stream.cc [modify] https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3/net/quic/chromium/quic_chromium_client_stream_test.cc [modify] https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3/net/quic/core/quic_spdy_stream.cc [modify] https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3/net/quic/core/quic_spdy_stream.h
,
Oct 6 2016
,
Oct 7 2016
Re-open this bug. CL #4 handled asynchronous read(), but it didn't handle the synchronous read() case. The problem is that when trailers are received, QuicChromiumClientStream posts an async task to notify delegate but synchronously call OnStreamFrame(FIN=true). If Read() is called after trailers are received but before the notification is delivered, Read() will synchronously return 0.
,
Oct 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ec0ed02ab197a731d3dcccdd7379c71a057353a1 commit ec0ed02ab197a731d3dcccdd7379c71a057353a1 Author: xunjieli <xunjieli@chromium.org> Date: Mon Oct 10 18:05:06 2016 QuicChromiumClientStream handle Read() after trailers are received but not yet delivered This CL changes QuicChromiumClientStream::Read() to return ERR_IO_PENDNG if trailers are received but not yet delivered. This CL additionally posts a OnDataAvailable task when trailers are delivered so as to notify delegate of the FIN flag. BUG= 652935 Review-Url: https://codereview.chromium.org/2405523002 Cr-Commit-Position: refs/heads/master@{#424183} [modify] https://crrev.com/ec0ed02ab197a731d3dcccdd7379c71a057353a1/net/quic/chromium/bidirectional_stream_quic_impl.cc [modify] https://crrev.com/ec0ed02ab197a731d3dcccdd7379c71a057353a1/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc [modify] https://crrev.com/ec0ed02ab197a731d3dcccdd7379c71a057353a1/net/quic/chromium/quic_chromium_client_stream.cc [modify] https://crrev.com/ec0ed02ab197a731d3dcccdd7379c71a057353a1/net/quic/chromium/quic_chromium_client_stream_test.cc
,
Oct 10 2016
I will wait for a day for the change to bake in canary before requesting merge into m55.
,
Oct 11 2016
Requesting merge approval for CL in #7. Thanks!
,
Oct 11 2016
Looks like OS-All, correct me if I'm wrong.
,
Oct 11 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Oct 11 2016
Please merge your change to M55 branch 2883 before 5:00 PM PT today (latest before 5:00 PM PT tomorrow, Wednesday (10/12/16)) so we can pick it up for this week M55 Dev release.Thank you.
,
Oct 11 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b031400ecbaf8e97f664e7e0de0ee699a2c8bab9 commit b031400ecbaf8e97f664e7e0de0ee699a2c8bab9 Author: xunjieli <xunjieli@chromium.org> Date: Tue Oct 11 18:27:06 2016 QuicChromiumClientStream handle Read() after trailers are received but not yet delivered This CL changes QuicChromiumClientStream::Read() to return ERR_IO_PENDNG if trailers are received but not yet delivered. This CL additionally posts a OnDataAvailable task when trailers are delivered so as to notify delegate of the FIN flag. BUG= 652935 NOPRESUBMIT=true NOTRY=true TBR=zhongyi@chromium.org Review-Url: https://codereview.chromium.org/2405523002 Cr-Commit-Position: refs/heads/master@{#424183} (cherry picked from commit ec0ed02ab197a731d3dcccdd7379c71a057353a1) Review-Url: https://codereview.chromium.org/2409273002 Cr-Commit-Position: refs/branch-heads/2883@{#37} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/b031400ecbaf8e97f664e7e0de0ee699a2c8bab9/net/quic/chromium/bidirectional_stream_quic_impl.cc [modify] https://crrev.com/b031400ecbaf8e97f664e7e0de0ee699a2c8bab9/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc [modify] https://crrev.com/b031400ecbaf8e97f664e7e0de0ee699a2c8bab9/net/quic/chromium/quic_chromium_client_stream.cc [modify] https://crrev.com/b031400ecbaf8e97f664e7e0de0ee699a2c8bab9/net/quic/chromium/quic_chromium_client_stream_test.cc
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7640faa369a7769b97719938c70e4e4910f476c3 commit 7640faa369a7769b97719938c70e4e4910f476c3 Author: xunjieli <xunjieli@chromium.org> Date: Thu Oct 06 00:45:42 2016 Remove OnDataAvailable() notification when trailers are to be delivered This CL used the change that is suggested by mpw@ in https://bugs.chromium.org/p/chromium/issues/detail?id=652935#c1. Specifically, this CL removes QuicChromiumClientStream::Delegate::OnDataAvailable() notification if there is no byte can be read and trailers are to be delivered. This CL adjusted unit tests to test the correct ordering of events. BUG= 652935 Review-Url: https://codereview.chromium.org/2393233002 Cr-Commit-Position: refs/heads/master@{#423357} [modify] https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3/net/quic/chromium/bidirectional_stream_quic_impl.cc [modify] https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc [modify] https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3/net/quic/chromium/quic_chromium_client_stream.cc [modify] https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3/net/quic/chromium/quic_chromium_client_stream_test.cc [modify] https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3/net/quic/core/quic_spdy_stream.cc [modify] https://crrev.com/7640faa369a7769b97719938c70e4e4910f476c3/net/quic/core/quic_spdy_stream.h
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b031400ecbaf8e97f664e7e0de0ee699a2c8bab9 commit b031400ecbaf8e97f664e7e0de0ee699a2c8bab9 Author: xunjieli <xunjieli@chromium.org> Date: Tue Oct 11 18:27:06 2016 QuicChromiumClientStream handle Read() after trailers are received but not yet delivered This CL changes QuicChromiumClientStream::Read() to return ERR_IO_PENDNG if trailers are received but not yet delivered. This CL additionally posts a OnDataAvailable task when trailers are delivered so as to notify delegate of the FIN flag. BUG= 652935 NOPRESUBMIT=true NOTRY=true TBR=zhongyi@chromium.org Review-Url: https://codereview.chromium.org/2405523002 Cr-Commit-Position: refs/heads/master@{#424183} (cherry picked from commit ec0ed02ab197a731d3dcccdd7379c71a057353a1) Review-Url: https://codereview.chromium.org/2409273002 Cr-Commit-Position: refs/branch-heads/2883@{#37} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/b031400ecbaf8e97f664e7e0de0ee699a2c8bab9/net/quic/chromium/bidirectional_stream_quic_impl.cc [modify] https://crrev.com/b031400ecbaf8e97f664e7e0de0ee699a2c8bab9/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc [modify] https://crrev.com/b031400ecbaf8e97f664e7e0de0ee699a2c8bab9/net/quic/chromium/quic_chromium_client_stream.cc [modify] https://crrev.com/b031400ecbaf8e97f664e7e0de0ee699a2c8bab9/net/quic/chromium/quic_chromium_client_stream_test.cc
,
Nov 4 2016
[Automated comment] removing mislabelled merge-merged-2840
,
Aug 24 2017
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by mpw@chromium.org
, Oct 5 2016Could this be addressed by having QuicChromiumClientStream::OnDataAvailable() check QuicSpdyStream::FinishedReadingTrailers() (or perform equivalent checks on fin_received() and trailers_delivered())? e.g. something like: void QuicChromiumClientStream::OnDataAvailable() { if (!FinishedReadingHeaders() || !headers_delivered_) { // Buffer the data in the sequencer until the headers have been read. return; } if (!sequencer().HasBytesToRead() && !(fin_received() || trailers_delivered())) { return; } // The delegate will read the data via a posted task, and // will be able to, potentially, read all data which has queued up. NotifyDelegateOfDataAvailableLater(); }