Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2918663002:
Clean up error handling in BidirectionalStreamQuicImpl::Start (Closed)
|
Created:
3 years, 6 months ago by Ryan Hamilton Modified:
3 years, 6 months ago Reviewers:
xunjieli CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up error handling in BidirectionalStreamQuicImpl::Start
Previously, NotifyError was called before delegate_ was assigned to
which meant that the delegate was not notified of any errors.
Review-Url: https://codereview.chromium.org/2918663002
Cr-Commit-Position: refs/heads/master@{#476192}
Committed: https://chromium.googlesource.com/chromium/src/+/12cdf8017a99e246e8acff7c3512fbd4406cafe0
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address comments #Patch Set 3 : Fix test #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 19 (11 generated)
rch@chromium.org changed reviewers: + xunjieli@chromium.org
https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... File net/quic/chromium/bidirectional_stream_quic_impl.cc (left): https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... net/quic/chromium/bidirectional_stream_quic_impl.cc:63: } Since both the handle's RequestStream and the session's RequestStream return an error if the session is closed, and the session handle is always valid, we don't need to check IsConnected(). https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... File net/quic/chromium/bidirectional_stream_quic_impl.cc (right): https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... net/quic/chromium/bidirectional_stream_quic_impl.cc:72: FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError, I'm pretty sure we need to do a PostTask here, otherwise we'd call NotifyError while under the callstack of Start(), which I think is problematic? https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... net/quic/chromium/bidirectional_stream_quic_impl.cc:75: ? ERR_QUIC_PROTOCOL_ERROR We could use |rv| instead of ERR_QUIC_PROTOCOL_ERROR if we wished. (It'd likely be ERR_CONNECTION_CLOSED). Your call.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... File net/quic/chromium/bidirectional_stream_quic_impl.cc (left): https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... net/quic/chromium/bidirectional_stream_quic_impl.cc:63: } On 2017/05/31 21:51:59, Ryan Hamilton wrote: > Since both the handle's RequestStream and the session's RequestStream return an > error if the session is closed, and the session handle is always valid, we don't > need to check IsConnected(). Acknowledged. https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... File net/quic/chromium/bidirectional_stream_quic_impl.cc (right): https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... net/quic/chromium/bidirectional_stream_quic_impl.cc:72: FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError, On 2017/05/31 21:51:59, Ryan Hamilton wrote: > I'm pretty sure we need to do a PostTask here, otherwise we'd call NotifyError > while under the callstack of Start(), which I think is problematic? Per offline discussion, I think the PostTask is not necessary because |delegate_| shouldn't call us back after being notified with an error. I am fine leaving it as it is since other methods have this problem as well. https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... net/quic/chromium/bidirectional_stream_quic_impl.cc:75: ? ERR_QUIC_PROTOCOL_ERROR On 2017/05/31 21:51:59, Ryan Hamilton wrote: > We could use |rv| instead of ERR_QUIC_PROTOCOL_ERROR if we wished. (It'd likely > be ERR_CONNECTION_CLOSED). Your call. SG. Let's use |rv| here.
Thanks! Landing now... https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... File net/quic/chromium/bidirectional_stream_quic_impl.cc (right): https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... net/quic/chromium/bidirectional_stream_quic_impl.cc:72: FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError, On 2017/05/31 23:40:12, xunjieli (Slow.until.6-5) wrote: > On 2017/05/31 21:51:59, Ryan Hamilton wrote: > > I'm pretty sure we need to do a PostTask here, otherwise we'd call NotifyError > > while under the callstack of Start(), which I think is problematic? > > Per offline discussion, I think the PostTask is not necessary because > |delegate_| shouldn't call us back after being notified with an error. I'm not worried about the delegate calling back into the BidiStream. Instead, if NotifyError() is called synchronously here, then the BidiStream will be reentrantly calling into the delegate while the delegate might not be expecting to be called. > I am fine leaving it as it is since other methods have this problem as well. SGTM. Let's clean up all of these in a follow up (after we've sorted out the crash bug fallout) https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... net/quic/chromium/bidirectional_stream_quic_impl.cc:75: ? ERR_QUIC_PROTOCOL_ERROR On 2017/05/31 23:40:12, xunjieli (Slow.until.6-5) wrote: > On 2017/05/31 21:51:59, Ryan Hamilton wrote: > > We could use |rv| instead of ERR_QUIC_PROTOCOL_ERROR if we wished. (It'd > likely > > be ERR_CONNECTION_CLOSED). Your call. > > SG. Let's use |rv| here. Done.
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/2918663002/#ps40001 (title: "Fix test")
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rch@chromium.org
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": 40001, "attempt_start_ts": 1496289692201960,
"parent_rev": "8f6a87ae5a06c8a93d64ddef451563414ddad9de", "commit_rev":
"12cdf8017a99e246e8acff7c3512fbd4406cafe0"}
Message was sent while issue was closed.
Description was changed from ========== Clean up error handling in BidirectionalStreamQuicImpl::Start Previously, NotifyError was called before delegate_ was assigned to which meant that the delegate was not notified of any errors. ========== to ========== Clean up error handling in BidirectionalStreamQuicImpl::Start Previously, NotifyError was called before delegate_ was assigned to which meant that the delegate was not notified of any errors. Review-Url: https://codereview.chromium.org/2918663002 Cr-Commit-Position: refs/heads/master@{#476192} Committed: https://chromium.googlesource.com/chromium/src/+/12cdf8017a99e246e8acff7c3512... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/12cdf8017a99e246e8acff7c3512...
Message was sent while issue was closed.
https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... File net/quic/chromium/bidirectional_stream_quic_impl.cc (right): https://codereview.chromium.org/2918663002/diff/1/net/quic/chromium/bidirecti... net/quic/chromium/bidirectional_stream_quic_impl.cc:72: FROM_HERE, base::Bind(&BidirectionalStreamQuicImpl::NotifyError, I thought about this some more yesterday. I think you are right. If Start() can call NotifyError() synchronously, the BidiStream (delegate of BidiStreamQuicImpl) might not be expecting that OnFailed() callback being called in Start(). If BidiStream accesses BidiStreamQuicImpl's states synchronously after calling BidiStreamQuicImpl::Start() (right now it doesn't), we will be in trouble. OnStreamReady() on line 80 can call |delegate_|::OnStreamReady() synchronously during Start(). I think that needs to be changed later on so we are consistent about not calling into |delegate_| synchronously? Sorry that this class is a mess! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
