Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873963003:
Add an async ReadInitialHeaders method to QuicChromiumClientStream::Handle (Closed)
|
Created:
3 years, 7 months ago by Ryan Hamilton Modified:
3 years, 7 months ago Reviewers:
xunjieli CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd an async ReadInitialHeaders method to QuicChromiumClientStream::Handle
This does not change OnClose or OnError and the callback is not yet invoked
on errors. That will happen after all the upcalls are replaced by async
methods.
BUG=716563
Review-Url: https://codereview.chromium.org/2873963003
Cr-Commit-Position: refs/heads/master@{#473458}
Committed: https://chromium.googlesource.com/chromium/src/+/fb47f71aa9f2ad207139f5bb03269eba31a0be67
Patch Set 1 #Patch Set 2 : Cleanup #
Total comments: 18
Patch Set 3 : Fix comments #
Total comments: 12
Patch Set 4 : Fix more comments #
Total comments: 7
Patch Set 5 : more #Patch Set 6 : Rebase #Dependent Patchsets: Messages
Total messages: 25 (13 generated)
rch@chromium.org changed reviewers: + xunjieli@chromium.org
https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/bidir... File net/quic/chromium/bidirectional_stream_quic_impl.cc (right): https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/bidir... net/quic/chromium/bidirectional_stream_quic_impl.cc:289: int rv = stream_->ReadInitialHeaders( nit: remove "int" before "rv" https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_stream.cc (right): https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:69: int rv; Looks like DeliverInitialiHeaders() can fail to initialize |rv|. Should we initialize |rv|? https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:70: if (!stream_->DeliverInitialHeaders(read_headers_buffer_, &rv)) If BidiStream or QuicHttpStream is gone but Handle is not, |read_headers_buffer_| will be invalid. Right now everything works because BidiStream/QuicHttpStream owns Handle, so |read_headers_buffer_| is guaranteed to be alive when Handle is. That's fragile because it also depends on destruction order. We could make Handle own the SpdyHeaderBlock, though that would result in having an extra accessor. https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:111: ScopedBoolSaver saver(&can_invoke_callbacks_, false); Why do we need to have this bool? I don't see any re-entrancy in this method. https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:115: int frame_len; same here. |frame_len| might not be initialized. https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_stream.h (right): https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.h:70: // Reads initial headers into |header_block| and returns the lenght of nit: typo in length https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.h:72: // returns ERR_IO_PENDING and will invokes |callback| asynchronously when nit: s/invokes/invoke
https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/bidir... File net/quic/chromium/bidirectional_stream_quic_impl.cc (right): https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/bidir... net/quic/chromium/bidirectional_stream_quic_impl.cc:289: int rv = stream_->ReadInitialHeaders( On 2017/05/10 17:54:27, xunjieli wrote: > nit: remove "int" before "rv" Done. https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_stream.cc (right): https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:69: int rv; On 2017/05/10 17:54:27, xunjieli wrote: > Looks like DeliverInitialiHeaders() can fail to initialize |rv|. Should we > initialize |rv|? I think that should only happen when Deliver returns false, in which case we don't use rv. But better safe than sorry! Done. https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:70: if (!stream_->DeliverInitialHeaders(read_headers_buffer_, &rv)) On 2017/05/10 17:54:27, xunjieli wrote: > If BidiStream or QuicHttpStream is gone but Handle is not, > |read_headers_buffer_| will be invalid. > Right now everything works because BidiStream/QuicHttpStream owns Handle, so > |read_headers_buffer_| is guaranteed to be alive when Handle is. That's fragile > because it also depends on destruction order. The Handle is by definition owned by "the caller" (bidi/http stream). They must outlive the handle. This is a common net async idiom, isn't it? If you call an async method on some object which has not yet completed by the time you are destroyed, you need to do one of two things: 1) delete that object (if you own it) 2) cancel the async operation In this case, because ownership is clear, we're doing #1. > We could make Handle own the SpdyHeaderBlock, though that would result in having > an extra accessor. I don't understand how that would work. Would the caller not pass in a SpdyHeaderBlock*? That's not the common net async read pattern. https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:111: ScopedBoolSaver saver(&can_invoke_callbacks_, false); On 2017/05/10 17:54:27, xunjieli wrote: > Why do we need to have this bool? I don't see any re-entrancy in this method. AH! Sorry, I meant to mention this in the CL description but forgot to. So this CL is one of 5 (I think). We need to add async support to: 1) Read Initial headers 2) Read body 3) Read trailers (I think this can always be synchronous) 4) Send headers (I think this can always be synchronous) 5) Send body (This is already async, but in the wrong place) Once we've done all of this, we can remove OnClose/OnError. In order to remove the OnError/OnClose up-call, the handle needs to know when it's ok to call the callbacks, and when it's not. This is not required for *this* CL, but I'm jumping through these hoops now in preparation for the later changes. (But I can skip them now if you prefer). For example, there might be a situation where a ReadBody() async call is pending when a WriteBody() call causes a write error, which shows up as Handle::OnError being called from the stream. Handle::OnError will notice that read_body_callback_ is set, and might be inclined to do read_body_callback_.Run(error), but that would be wrong. The right thing to do would be to return error from WriteBody(). So this "can_invoke_callbacks_" member is the trigger to avoid invoking the callback. But I can totally nuke this if for now if you prefer, since it's speculative at this point. Your call. https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:115: int frame_len; On 2017/05/10 17:54:27, xunjieli wrote: > same here. |frame_len| might not be initialized. Done. https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_stream.h (right): https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.h:70: // Reads initial headers into |header_block| and returns the lenght of On 2017/05/10 17:54:27, xunjieli wrote: > nit: typo in length Done. https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.h:72: // returns ERR_IO_PENDING and will invokes |callback| asynchronously when On 2017/05/10 17:54:27, xunjieli wrote: > nit: s/invokes/invoke Done.
Looks great. Just two more suggestions on the tests and then I am ready to sign off. There is a typo in the CL title. s/and/an https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_stream.cc (right): https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:70: if (!stream_->DeliverInitialHeaders(read_headers_buffer_, &rv)) On 2017/05/10 18:26:40, Ryan Hamilton wrote: > On 2017/05/10 17:54:27, xunjieli wrote: > > If BidiStream or QuicHttpStream is gone but Handle is not, > > |read_headers_buffer_| will be invalid. > > Right now everything works because BidiStream/QuicHttpStream owns Handle, so > > |read_headers_buffer_| is guaranteed to be alive when Handle is. That's > fragile > > because it also depends on destruction order. > > The Handle is by definition owned by "the caller" (bidi/http stream). They must > outlive the handle. This is a common net async idiom, isn't it? If you call an > async method on some object which has not yet completed by the time you are > destroyed, you need to do one of two things: > > 1) delete that object (if you own it) > 2) cancel the async operation > > In this case, because ownership is clear, we're doing #1. > > > We could make Handle own the SpdyHeaderBlock, though that would result in > having > > an extra accessor. > > I don't understand how that would work. Would the caller not pass in a > SpdyHeaderBlock*? That's not the common net async read pattern. Acknowledged. The ownership model takes a while to get used to. But I like the lifetime guarantee that now we know we are going to enforce it :) https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:111: ScopedBoolSaver saver(&can_invoke_callbacks_, false); On 2017/05/10 18:26:40, Ryan Hamilton wrote: > On 2017/05/10 17:54:27, xunjieli wrote: > > Why do we need to have this bool? I don't see any re-entrancy in this method. > > AH! Sorry, I meant to mention this in the CL description but forgot to. So this > CL is one of 5 (I think). We need to add async support to: > > 1) Read Initial headers > 2) Read body > 3) Read trailers (I think this can always be synchronous) > 4) Send headers (I think this can always be synchronous) > 5) Send body (This is already async, but in the wrong place) > > Once we've done all of this, we can remove OnClose/OnError. In order to remove > the OnError/OnClose up-call, the handle needs to know when it's ok to call the > callbacks, and when it's not. This is not required for *this* CL, but I'm > jumping through these hoops now in preparation for the later changes. (But I can > skip them now if you prefer). For example, there might be a situation where a > ReadBody() async call is pending when a WriteBody() call causes a write error, > which shows up as Handle::OnError being called from the stream. Handle::OnError > will notice that read_body_callback_ is set, and might be inclined to do > read_body_callback_.Run(error), but that would be wrong. The right thing to do > would be to return error from WriteBody(). So this "can_invoke_callbacks_" > member is the trigger to avoid invoking the callback. > > But I can totally nuke this if for now if you prefer, since it's speculative at > this point. Your call. I see. That makes sense. Let's omit this in this CL then. I would like to re-evaluate this part when we run into the situation that you described and make sure we have tests covering the sync-failure code paths. Mostly is that I wanted to make sure I understand how the pieces fit together :) Thanks! https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_stream.cc (right): https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:280: void QuicChromiumClientStream::Handle::SetCallback( Could you inline this once you omit ScopedBoolSaver? https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... File net/quic/chromium/quic_http_stream_test.cc (right): https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1427: // In the course of processing this packet, the QuicHttpStream close itself. This test is no longer testing what it claims to do. The response header packet will be processed by QuicHttpStream, right? Could you rework this test so that the QuicHttpStream is destroyed before response header packet? One suggestion: When ReadResponseHeaders returns ERR_IO_PENDING, invoke CloseStream() right away rather than doing CloseStream() as a completion callback. https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1476: base::Bind(&QuicHttpStreamTest::CloseStream, This test doesn't need the CloseStream/"Close early" logic. We could use |callback_.callback()| and wait for callback_ to complete. https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1493: // Zero since the stream is closed before processing the headers. The comment is no longer accurate. Could you update this line? https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1494: EXPECT_EQ(31, stream_->GetTotalReceivedBytes()); Can we get "31" from ConstructResponseHeadersPacket() ?
Description was changed from ========== Add and async ReadInitialHeaders method to QuicChromiumClientStream::Handle This does not change OnClose or OnError and the callback is not yet invoked on errors. That will happen after all the upcalls are replaced by async methods. BUG=716563 ========== to ========== Add an async ReadInitialHeaders method to QuicChromiumClientStream::Handle This does not change OnClose or OnError and the callback is not yet invoked on errors. That will happen after all the upcalls are replaced by async methods. BUG=716563 ==========
Thanks! https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_stream.cc (right): https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:70: if (!stream_->DeliverInitialHeaders(read_headers_buffer_, &rv)) On 2017/05/10 18:58:08, xunjieli wrote: > On 2017/05/10 18:26:40, Ryan Hamilton wrote: > > On 2017/05/10 17:54:27, xunjieli wrote: > > > If BidiStream or QuicHttpStream is gone but Handle is not, > > > |read_headers_buffer_| will be invalid. > > > Right now everything works because BidiStream/QuicHttpStream owns Handle, so > > > |read_headers_buffer_| is guaranteed to be alive when Handle is. That's > > fragile > > > because it also depends on destruction order. > > > > The Handle is by definition owned by "the caller" (bidi/http stream). They > must > > outlive the handle. This is a common net async idiom, isn't it? If you call an > > async method on some object which has not yet completed by the time you are > > destroyed, you need to do one of two things: > > > > 1) delete that object (if you own it) > > 2) cancel the async operation > > > > In this case, because ownership is clear, we're doing #1. > > > > > We could make Handle own the SpdyHeaderBlock, though that would result in > > having > > > an extra accessor. > > > > I don't understand how that would work. Would the caller not pass in a > > SpdyHeaderBlock*? That's not the common net async read pattern. > > Acknowledged. The ownership model takes a while to get used to. But I like the > lifetime guarantee that now we know we are going to enforce it :) *fingers crossed*! https://codereview.chromium.org/2873963003/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:111: ScopedBoolSaver saver(&can_invoke_callbacks_, false); On 2017/05/10 18:58:08, xunjieli wrote: > On 2017/05/10 18:26:40, Ryan Hamilton wrote: > > On 2017/05/10 17:54:27, xunjieli wrote: > > > Why do we need to have this bool? I don't see any re-entrancy in this > method. > > > > AH! Sorry, I meant to mention this in the CL description but forgot to. So > this > > CL is one of 5 (I think). We need to add async support to: > > > > 1) Read Initial headers > > 2) Read body > > 3) Read trailers (I think this can always be synchronous) > > 4) Send headers (I think this can always be synchronous) > > 5) Send body (This is already async, but in the wrong place) > > > > Once we've done all of this, we can remove OnClose/OnError. In order to remove > > the OnError/OnClose up-call, the handle needs to know when it's ok to call the > > callbacks, and when it's not. This is not required for *this* CL, but I'm > > jumping through these hoops now in preparation for the later changes. (But I > can > > skip them now if you prefer). For example, there might be a situation where a > > ReadBody() async call is pending when a WriteBody() call causes a write error, > > which shows up as Handle::OnError being called from the stream. > Handle::OnError > > will notice that read_body_callback_ is set, and might be inclined to do > > read_body_callback_.Run(error), but that would be wrong. The right thing to do > > would be to return error from WriteBody(). So this "can_invoke_callbacks_" > > member is the trigger to avoid invoking the callback. > > > > But I can totally nuke this if for now if you prefer, since it's speculative > at > > this point. Your call. > > I see. That makes sense. Let's omit this in this CL then. I would like to > re-evaluate this part when we run into the situation that you described and make > sure we have tests covering the sync-failure code paths. Mostly is that I wanted > to make sure I understand how the pieces fit together :) > Thanks! SGTM. Done. https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_stream.cc (right): https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_stream.cc:280: void QuicChromiumClientStream::Handle::SetCallback( On 2017/05/10 18:58:08, xunjieli wrote: > Could you inline this once you omit ScopedBoolSaver? Indeed. Done. https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... File net/quic/chromium/quic_http_stream_test.cc (right): https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1427: // In the course of processing this packet, the QuicHttpStream close itself. On 2017/05/10 18:58:08, xunjieli wrote: > This test is no longer testing what it claims to do. > The response header packet will be processed by QuicHttpStream, right? Could you > rework this test so that the QuicHttpStream is destroyed before response header > packet? > > One suggestion: > When ReadResponseHeaders returns ERR_IO_PENDING, invoke CloseStream() right away > rather than doing CloseStream() as a completion callback. The test says that the QuicHttpStream will be close itself while the headers packet is being processed, and I think that's what the test doest. It start s a read before the packet arrives. Then when the packet arrives, the headers are handed to the stream, and the stream invokes the OnInitialHeaders method of the handle, which in turn invokes the read callback on the QuicHttpStream which closes the stream. I think that matches up. (ProcessPacket hand the packet to the QuicConnection, not the QuicHttpStream) https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1476: base::Bind(&QuicHttpStreamTest::CloseStream, On 2017/05/10 18:58:08, xunjieli wrote: > This test doesn't need the CloseStream/"Close early" logic. We could use > |callback_.callback()| and wait for callback_ to complete. Actually the test does rely on the stream closing currently because of the 3rd write. But that doesn't seem important to the test, so I'll nuke it. Done. https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1493: // Zero since the stream is closed before processing the headers. On 2017/05/10 18:58:08, xunjieli wrote: > The comment is no longer accurate. Could you update this line? Done. https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1494: EXPECT_EQ(31, stream_->GetTotalReceivedBytes()); On 2017/05/10 18:58:08, xunjieli wrote: > Can we get "31" from ConstructResponseHeadersPacket() ? Done.
lgtm mod one nit below. https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... File net/quic/chromium/quic_http_stream_test.cc (right): https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1427: // In the course of processing this packet, the QuicHttpStream close itself. On 2017/05/10 19:33:46, Ryan Hamilton wrote: > On 2017/05/10 18:58:08, xunjieli wrote: > > This test is no longer testing what it claims to do. > > The response header packet will be processed by QuicHttpStream, right? Could > you > > rework this test so that the QuicHttpStream is destroyed before response > header > > packet? > > > > One suggestion: > > When ReadResponseHeaders returns ERR_IO_PENDING, invoke CloseStream() right > away > > rather than doing CloseStream() as a completion callback. > > The test says that the QuicHttpStream will be close itself while the headers > packet is being processed, and I think that's what the test doest. It start s a > read before the packet arrives. Then when the packet arrives, the headers are > handed to the stream, and the stream invokes the OnInitialHeaders method of the > handle, which in turn invokes the read callback on the QuicHttpStream which > closes the stream. I think that matches up. > > (ProcessPacket hand the packet to the QuicConnection, not the QuicHttpStream) Acknowledged. The comment that confused me is line 1438. The previous test is checking that the QuicHttpStream processed 0 header bytes. Could you update the comment? https://codereview.chromium.org/2873963003/diff/60001/net/quic/chromium/quic_... File net/quic/chromium/quic_http_stream_test.cc (right): https://codereview.chromium.org/2873963003/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1420: EXPECT_THAT(stream_->ReadResponseHeaders( optional: |stream_| is an AutoClosingStream(). Instead of making a QuicHttpStreamTest::CloseStream method, we could just bind OnInitialHeadersAvailable method. https://codereview.chromium.org/2873963003/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1438: // Zero since the stream is closed before processing the headers. nit: Let's update the comment here. https://codereview.chromium.org/2873963003/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1439: EXPECT_EQ(31, stream_->GetTotalReceivedBytes()); nit: static_cast<int64_t>(response_size)
https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... File net/quic/chromium/quic_http_stream_test.cc (right): https://codereview.chromium.org/2873963003/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1427: // In the course of processing this packet, the QuicHttpStream close itself. On 2017/05/10 19:46:58, xunjieli wrote: > On 2017/05/10 19:33:46, Ryan Hamilton wrote: > > On 2017/05/10 18:58:08, xunjieli wrote: > > > This test is no longer testing what it claims to do. > > > The response header packet will be processed by QuicHttpStream, right? Could > > you > > > rework this test so that the QuicHttpStream is destroyed before response > > header > > > packet? > > > > > > One suggestion: > > > When ReadResponseHeaders returns ERR_IO_PENDING, invoke CloseStream() right > > away > > > rather than doing CloseStream() as a completion callback. > > > > The test says that the QuicHttpStream will be close itself while the headers > > packet is being processed, and I think that's what the test doest. It start s > a > > read before the packet arrives. Then when the packet arrives, the headers are > > handed to the stream, and the stream invokes the OnInitialHeaders method of > the > > handle, which in turn invokes the read callback on the QuicHttpStream which > > closes the stream. I think that matches up. > > > > (ProcessPacket hand the packet to the QuicConnection, not the QuicHttpStream) > > Acknowledged. The comment that confused me is line 1438. The previous test is > checking that the QuicHttpStream processed 0 header bytes. Could you update the > comment? Ah good point. Yeah, that's confusing. Done. https://codereview.chromium.org/2873963003/diff/60001/net/quic/chromium/quic_... File net/quic/chromium/quic_http_stream_test.cc (right): https://codereview.chromium.org/2873963003/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1420: EXPECT_THAT(stream_->ReadResponseHeaders( On 2017/05/10 19:46:58, xunjieli wrote: > optional: > |stream_| is an AutoClosingStream(). > Instead of making a QuicHttpStreamTest::CloseStream method, we could just bind > OnInitialHeadersAvailable method. OnInitialHeadersAvailable is a method on the handle, but is no longer a method of the QuicHttpStream (of which AutoClosingStream is a subclass). Since this is a test of QuicHttpStream, if I hooked into the Handle's OnInitialHeadersAvailable, it wouldn't be testing the right code path, I don't think. (We need QuicHttpStream::Close() to run in this test). The old version of this test did what you proposed, but that worked because OnInitialHeadersAvailable used to be a method of QuicHttpStream (via QuicChromiumClientStream::Delegate). But that method is being removed in this CL. One we've fully async'd the Handle, we'll be able to completely nuke AutoClosingStream, I hope. Sound plausible? https://codereview.chromium.org/2873963003/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1438: // Zero since the stream is closed before processing the headers. On 2017/05/10 19:46:58, xunjieli wrote: > nit: Let's update the comment here. Done. https://codereview.chromium.org/2873963003/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1439: EXPECT_EQ(31, stream_->GetTotalReceivedBytes()); On 2017/05/10 19:46:58, xunjieli wrote: > nit: static_cast<int64_t>(response_size) Done.
The CQ bit was checked by rch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2873963003/diff/60001/net/quic/chromium/quic_... File net/quic/chromium/quic_http_stream_test.cc (right): https://codereview.chromium.org/2873963003/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream_test.cc:1420: EXPECT_THAT(stream_->ReadResponseHeaders( On 2017/05/10 19:57:55, Ryan Hamilton wrote: > On 2017/05/10 19:46:58, xunjieli wrote: > > optional: > > |stream_| is an AutoClosingStream(). > > Instead of making a QuicHttpStreamTest::CloseStream method, we could just bind > > OnInitialHeadersAvailable method. > > OnInitialHeadersAvailable is a method on the handle, but is no longer a method > of the QuicHttpStream (of which AutoClosingStream is a subclass). Since this is > a test of QuicHttpStream, if I hooked into the Handle's > OnInitialHeadersAvailable, it wouldn't be testing the right code path, I don't > think. (We need QuicHttpStream::Close() to run in this test). > > The old version of this test did what you proposed, but that worked because > OnInitialHeadersAvailable used to be a method of QuicHttpStream (via > QuicChromiumClientStream::Delegate). But that method is being removed in this > CL. > > One we've fully async'd the Handle, we'll be able to completely nuke > AutoClosingStream, I hope. > > Sound plausible? Acknowledged. SGTM!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2873963003/#ps80001 (title: "more")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2873963003/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495332257153750,
"parent_rev": "bc62dfeaf37c83a79f393cb536fb22b39ea87097", "commit_rev":
"fb47f71aa9f2ad207139f5bb03269eba31a0be67"}
Message was sent while issue was closed.
Description was changed from ========== Add an async ReadInitialHeaders method to QuicChromiumClientStream::Handle This does not change OnClose or OnError and the callback is not yet invoked on errors. That will happen after all the upcalls are replaced by async methods. BUG=716563 ========== to ========== Add an async ReadInitialHeaders method to QuicChromiumClientStream::Handle This does not change OnClose or OnError and the callback is not yet invoked on errors. That will happen after all the upcalls are replaced by async methods. BUG=716563 Review-Url: https://codereview.chromium.org/2873963003 Cr-Commit-Position: refs/heads/master@{#473458} Committed: https://chromium.googlesource.com/chromium/src/+/fb47f71aa9f2ad207139f5bb0326... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/fb47f71aa9f2ad207139f5bb0326... |
