Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1326503003:
Added a net::BidirectionalStream to expose a bidirectional streaming interface (Closed)
DescriptionAdded a net::BidirectionalStream to expose a bidirectional streaming interface
This CL adds a BidirectionalStream on top of SpdyStream to
expose a bidirectional streaming interface. This CL also
modifies HttpStreamFactoryImplJob to create BidirectionalStream.
BUG=516342
Committed: https://crrev.com/11834f05b42d7c5e8cc7bd5774033db3773627a5
Cr-Commit-Position: refs/heads/master@{#366541}
Patch Set 1 : #
Total comments: 29
Patch Set 2 : Address Comments #Patch Set 3 : Pass through priority and netlog #
Total comments: 32
Patch Set 4 : Address Comments #
Total comments: 30
Patch Set 5 : Address Misha's and Matt's comments #
Total comments: 2
Patch Set 6 : Avoid inlining #
Total comments: 31
Patch Set 7 : Address Matt's comments #Patch Set 8 : Make the wrapper class own the stream #
Total comments: 32
Patch Set 9 : Address Matt's and Misha's Comments. Renamed a bunch of classes #Patch Set 10 : Rebased #
Total comments: 13
Patch Set 11 : Address Misha's comments #
Total comments: 2
Patch Set 12 : Delay OnClose properly #Patch Set 13 : Remove unneeded return value #
Total comments: 30
Patch Set 14 : Address Matt's comments #Patch Set 15 : Self review #Patch Set 16 : Test read buffering #Patch Set 17 : Get rid of Delegate::OnClose #
Total comments: 6
Patch Set 18 : Use SPDY_BIDIRECTIONAL_STREAM #
Total comments: 2
Patch Set 19 : Fix bug and added a test #
Total comments: 10
Patch Set 20 : Address Misha's comments #
Total comments: 70
Patch Set 21 : Address Matt's comments #
Total comments: 69
Patch Set 22 : Address comments and moved tests #
Total comments: 14
Patch Set 23 : Address Misha's comments #
Total comments: 6
Patch Set 24 : Document requirement on destruction #Patch Set 25 : Get rid of DeterministicSocketData #
Total comments: 54
Patch Set 26 : Added tests for canceling and propagating error #Patch Set 27 : Address Misha's comments #Patch Set 28 : Fix compile #Patch Set 29 : Use private inheritance #
Total comments: 42
Patch Set 30 : Added tests and a compile flag #Patch Set 31 : Address Matt's comments #
Total comments: 10
Patch Set 32 : Fix GN #Patch Set 33 : Address Misha's comments #
Total comments: 6
Patch Set 34 : Use std::move #
Total comments: 15
Patch Set 35 : Address Misha's comments #
Total comments: 3
Patch Set 36 : Self review #
Total comments: 8
Patch Set 37 : Address Misha's comments #
Total comments: 3
Messages
Total messages: 148 (40 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
xunjieli@chromium.org changed reviewers: + bnc@chromium.org, mef@chromium.org
Hi Misha, Here's the CL for the net::BidirectionalStream implementation. It is still pretty much a draft right now, but I'd love some initial feedbacks. Since Bence is traveling, could you do initial round of review? To make QUIC work, we will need another layer of abstraction, which isn't added in this CL to make the change small. Let me know what you think. (Tests for the bidi stream is in net/http/bidirectional_stream_unittest.cc, and net/http/http_stream_factory_impl_unittest.cc)
Patchset #1 (id:100001) has been deleted
On 2015/09/30 15:33:45, xunjieli wrote: > Hi Misha, > > Here's the CL for the net::BidirectionalStream implementation. It is still > pretty much a draft right now, but I'd love some initial feedbacks. Since Bence > is traveling, could you do initial round of review? > To make QUIC work, we will need another layer of abstraction, which isn't added > in this CL to make the change small. > > Let me know what you think. > > (Tests for the bidi stream is in > net/http/bidirectional_stream_unittest.cc, and > net/http/http_stream_factory_impl_unittest.cc) Very cool, I'll take a look!
Some comments from the first glance. https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream.cc:80: bool end_stream = (request_info_->method == "GET"); I wonder what should happen with different request methods, like PUT or HEAD? https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream.h:18: class BidirectionalStream : public SpdyStream::Delegate { I think BidirectionalStream should be an abstract base class, and BidirectionalSpdyStream should be its H2 - specific implementation. https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream.h:35: virtual void OnDataReceived(scoped_ptr<SpdyBuffer> buffer) = 0; I think it should be similar to onReadCompleted, called back when data is read from internal SpdyReadQueue into buffer passed by caller to 'ReadData'. https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream.h:60: // Sends data. I think it needs 'ReadData' counterpart, which in Spdy case will read from internal SpdyReadQueue. https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:31: HttpStreamFactoryImpl::Request::Request( why do we need to duplicate constructors? Can we add parameter to existing one? https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:324: } else if (for_bidirectional_) { Does this only work for new spdy session? https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:35: WebSocketHandshakeStreamBase::CreateHelper* Would it make sense to add BidirectionalStream::CreateHelper or somesuch in case if bidirectional stream is needed?
https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:146: scoped_ptr<SpdyFrame> resp(spdy_util_.ConstructSpdyGetSynReply(NULL, 0, 1)); Consider passing some extra_headers to ConstructSpdyGetSynReply and then EXPECT_EQ them against delegate_->response_headers_ (so that not only trailers but also headers are tested). https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:187: scoped_ptr<SpdyFrame> resp(spdy_util_.ConstructSpdyPostSynReply(NULL, 0)); Same thing here. https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:25: class BidirectionalStream; Why do you need this? https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:402: request_->OnNewSpdySessionReady(this, nullptr /** spdy_http_stream */, I think the syntax most often used is /*spdy_http_stream=*/ nullptr https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:408: nullptr /** bidirectional_stream */, /*bidirectional_stream=*/ nullptr https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1285: EXPECT_TRUE(nullptr == waiter.websocket_stream()); I think EXPECT_FALSE(waiter.websocket_stream()) is preferable. https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1287: ASSERT_TRUE(nullptr != waiter.bidirectional_stream()); And maybe ASSERT_TRUE(waiter.bidirectional_stream()) here.
Thanks for the feedback! PTAL. https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream.cc:80: bool end_stream = (request_info_->method == "GET"); On 2015/09/30 16:18:16, mef wrote: > I wonder what should happen with different request methods, like PUT or HEAD? I am not sure. Should we limit the use to POST and GET for now? Since that is probably our test server supports, and what our embedders want right now. https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream.h:18: class BidirectionalStream : public SpdyStream::Delegate { On 2015/09/30 16:18:16, mef wrote: > I think BidirectionalStream should be an abstract base class, and > BidirectionalSpdyStream should be its H2 - specific implementation. Done. https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream.h:35: virtual void OnDataReceived(scoped_ptr<SpdyBuffer> buffer) = 0; On 2015/09/30 16:18:16, mef wrote: > I think it should be similar to onReadCompleted, called back when data is read > from internal SpdyReadQueue into buffer passed by caller to 'ReadData'. Done. https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream.h:60: // Sends data. On 2015/09/30 16:18:16, mef wrote: > I think it needs 'ReadData' counterpart, which in Spdy case will read from > internal SpdyReadQueue. Done. https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:146: scoped_ptr<SpdyFrame> resp(spdy_util_.ConstructSpdyGetSynReply(NULL, 0, 1)); On 2015/10/01 05:29:28, Bence wrote: > Consider passing some extra_headers to ConstructSpdyGetSynReply and then > EXPECT_EQ them against delegate_->response_headers_ (so that not only trailers > but also headers are tested). Done. Good idea. Thanks! https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:187: scoped_ptr<SpdyFrame> resp(spdy_util_.ConstructSpdyPostSynReply(NULL, 0)); On 2015/10/01 05:29:28, Bence wrote: > Same thing here. Done. https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl.h (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl.h:25: class BidirectionalStream; On 2015/10/01 05:29:28, Bence wrote: > Why do you need this? Thanks for catching that. Done. https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:402: request_->OnNewSpdySessionReady(this, nullptr /** spdy_http_stream */, On 2015/10/01 05:29:28, Bence wrote: > I think the syntax most often used is > /*spdy_http_stream=*/ nullptr Done. https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:408: nullptr /** bidirectional_stream */, On 2015/10/01 05:29:28, Bence wrote: > /*bidirectional_stream=*/ nullptr Done. https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:31: HttpStreamFactoryImpl::Request::Request( On 2015/09/30 16:18:16, mef wrote: > why do we need to duplicate constructors? Can we add parameter to existing one? Done. https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:324: } else if (for_bidirectional_) { On 2015/09/30 16:18:16, mef wrote: > Does this only work for new spdy session? It also works for existing spdy session if HttpStreamFactoryImplJob finds one (see http_stream_factory_impl_job.cc). https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.h:35: WebSocketHandshakeStreamBase::CreateHelper* On 2015/09/30 16:18:16, mef wrote: > Would it make sense to add BidirectionalStream::CreateHelper or somesuch in case > if bidirectional stream is needed? I am not sure what the CreateHelper should do. WebSocket has one as it needs it hold a data key. Maybe when we add QUIC's version of bidi stream, we can have a helper to create the specific stream? I don't want to add it now since I don't have a clear idea of how it'd be useful. https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1285: EXPECT_TRUE(nullptr == waiter.websocket_stream()); On 2015/10/01 05:29:28, Bence wrote: > I think EXPECT_FALSE(waiter.websocket_stream()) is preferable. Done. https://codereview.chromium.org/1326503003/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:1287: ASSERT_TRUE(nullptr != waiter.bidirectional_stream()); On 2015/10/01 05:29:28, Bence wrote: > And maybe ASSERT_TRUE(waiter.bidirectional_stream()) here. Done.
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org
Some preliminary comments. Haven't really dug in yet https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:9: #include "base/memory/weak_ptr.h" Not needed https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:27: Delegate() {} nit: Blank line after constructor. https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:43: virtual void OnDataSent() = 0; This is called when all data from the last SendData call is sent? https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:73: virtual void SendData(IOBuffer* data, int length, bool end_stream) = 0; I assume it's not legal to call again until OnDataSent is called? https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory.h:238: const BoundNetLog& net_log) = 0; Unlike the two above methods, BidirectionalStreams are top level objects, which makes them rather different, from a logging standpoint. For normal requests, the BoundNetLog comes from the NetworkTransaction (Actually, the URLRequest created it). The HttpStreamFactoryImpl::Job uses the BoundNetLog to associate the job with the event that created it. When done, the NetworkTransaction also passes the same BoundNetLog to the BidirectionalStream. What's different here is the BidirectionalStreams this method creates are the top level object. Note also that this class is "NET_EXPORT_PRIVATE" - it's not currently used by anything outside net. So what does that all mean? We still need to figure out how we want these to be created. We could create a higher level class which creates and wraps the bidirectionalstream, and returns an error if it fails to create one. This does need a silly wrapper class, but it has some nice features - if the top level object is destroyed, we can just cancel the stream request, for instance. Or we could do something like "NetworkSession::CreateBidirectionStream(RequestInfo [Or components of it], priority, callback);" and then make the embedder responsible for starting it. The NetworkSession would create the stream and create a BoundNetLog for it. Or we could do something like "NetworkSession::CreateBidirectionStream(RequestInfo, priority, BidirectionalStream::Delegate);" This is kind of weird in that we'd be calling the stream's delegate's OnFailed callback in the case we didn't even create a stream. The latter two methods would also need a way to cancel the request (Maybe create a BidirecitonalStreamRequest object, which could be destroyed?). I'm leaning a little towards the first option - it's more like how URLRequest works, and discourages playing with the NetworkSession directly, which I think are nice features.
https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/120001/net/http/bidirectional... net/http/bidirectional_stream.cc:80: bool end_stream = (request_info_->method == "GET"); On 2015/10/01 18:41:16, xunjieli wrote: > On 2015/09/30 16:18:16, mef wrote: > > I wonder what should happen with different request methods, like PUT or HEAD? > > I am not sure. Should we limit the use to POST and GET for now? Since that is > probably our test server supports, and what our embedders want right now. Yeah, I think it is fine to keep this for now. https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:49: // called after this. Add a comment about values of |status|. https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:75: // TODO(xunjieli): implement a method to do flow control. Do we also need a method to explicitly close / cancel stream from the client side? For example if timeout has expired waiting for pending Send or Read? https://codereview.chromium.org/1326503003/diff/180001/net/spdy/bidirectional... File net/spdy/bidirectional_spdy_stream.cc (right): https://codereview.chromium.org/1326503003/diff/180001/net/spdy/bidirectional... net/spdy/bidirectional_spdy_stream.cc:116: // Complete remaining buffered read. what if there is no pending read?
A couple more comments https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:85: const BoundNetLog& net_log) { DCHECK the scheme is HTTPS? https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:88: net_log, true /** for_bidirectional */); I'm no HTTP2 expert, but it's my understanding that an HTTP/1.x server to advertise another server for HTTP2, in which case, the alternative job in HttpStreamFactoryImpl::RequestStreamInternal will actually produce an HTTP2 session, while the main job may (?) produce an HTTP/1.x one. The life of a URLRequest doc gets this wrong (Been meaning to fix it, but I don't have a great understanding of this code). https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:282: // BidirectionalStream. This seems the wrong place for this comment. Maybe in RequestBidirectionalStream above? https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:365: DCHECK(!stream_factory_->for_websockets_); Think the latter two of these make more sense as DCHECKs in StartInternal (Know you're copying from OnWebSocketHandshakeStreamReadyCallback, but think that gets things wrong there, too) https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:597: } else if (for_bidirectional_) { Hrm...Should we fail earlier for non-HTTPS/HTTP2 requests? https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:1185: int HttpStreamFactoryImpl::Job::SetSpdyHttpStream( This method needs to be renamed. https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:310: } else if (for_bidirectional_) { Maybe add DCHECK(bidirectional_stream); DCHECK(!stream); And the opposite in the other branch?
Patchset #5 (id:200001) has been deleted
Thanks for the reviews! I uploaded a new patch to address the comments. PTAL. https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:9: #include "base/memory/weak_ptr.h" On 2015/10/07 18:43:33, mmenke wrote: > Not needed Done. https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:27: Delegate() {} On 2015/10/07 18:43:33, mmenke wrote: > nit: Blank line after constructor. Done. https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:43: virtual void OnDataSent() = 0; On 2015/10/07 18:43:33, mmenke wrote: > This is called when all data from the last SendData call is sent? I think it is called for every SendData. https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:49: // called after this. On 2015/10/07 23:44:56, mef wrote: > Add a comment about values of |status|. Done. https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:73: virtual void SendData(IOBuffer* data, int length, bool end_stream) = 0; On 2015/10/07 18:43:33, mmenke wrote: > I assume it's not legal to call again until OnDataSent is called? Yes, that's my understanding too. I added a comment. https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:75: // TODO(xunjieli): implement a method to do flow control. On 2015/10/07 23:44:56, mef wrote: > Do we also need a method to explicitly close / cancel stream from the client > side? For example if timeout has expired waiting for pending Send or Read? Done. https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory.h:238: const BoundNetLog& net_log) = 0; On 2015/10/07 18:43:33, mmenke wrote: > Unlike the two above methods, BidirectionalStreams are top level objects, which > makes them rather different, from a logging standpoint. For normal requests, > the BoundNetLog comes from the NetworkTransaction (Actually, the URLRequest > created it). The HttpStreamFactoryImpl::Job uses the BoundNetLog to associate > the job with the event that created it. When done, the NetworkTransaction also > passes the same BoundNetLog to the BidirectionalStream. > > What's different here is the BidirectionalStreams this method creates are the > top level object. Note also that this class is "NET_EXPORT_PRIVATE" - it's not > currently used by anything outside net. > > So what does that all mean? We still need to figure out how we want these to be > created. > > We could create a higher level class which creates and wraps the > bidirectionalstream, and returns an error if it fails to create one. This does > need a silly wrapper class, but it has some nice features - if the top level > object is destroyed, we can just cancel the stream request, for instance. > > Or we could do something like > "NetworkSession::CreateBidirectionStream(RequestInfo [Or components of it], > priority, callback);" and then make the embedder responsible for starting it. > The NetworkSession would create the stream and create a BoundNetLog for it. > > Or we could do something like > "NetworkSession::CreateBidirectionStream(RequestInfo, priority, > BidirectionalStream::Delegate);" This is kind of weird in that we'd be calling > the stream's delegate's OnFailed callback in the case we didn't even create a > stream. > > The latter two methods would also need a way to cancel the request (Maybe create > a BidirecitonalStreamRequest object, which could be destroyed?). I'm leaning a > little towards the first option - it's more like how URLRequest works, and > discourages playing with the NetworkSession directly, which I think are nice > features. Talked to Matt offline on this. I added a wrapper class to address this issues brought up in this comment. https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:85: const BoundNetLog& net_log) { On 2015/10/08 19:31:53, mmenke wrote: > DCHECK the scheme is HTTPS? Done. https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:88: net_log, true /** for_bidirectional */); On 2015/10/08 19:31:53, mmenke wrote: > I'm no HTTP2 expert, but it's my understanding that an HTTP/1.x server to > advertise another server for HTTP2, in which case, the alternative job in > HttpStreamFactoryImpl::RequestStreamInternal will actually produce an HTTP2 > session, while the main job may (?) produce an HTTP/1.x one. The life of a > URLRequest doc gets this wrong (Been meaning to fix it, but I don't have a great > understanding of this code). Acknowledged. Here we don't use RequestStreamInternal. I was assuming that gRPC will issue the request directly to a HTTP/2 server. https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:282: // BidirectionalStream. On 2015/10/08 19:31:53, mmenke wrote: > This seems the wrong place for this comment. Maybe in > RequestBidirectionalStream above? Done. https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:365: DCHECK(!stream_factory_->for_websockets_); On 2015/10/08 19:31:53, mmenke wrote: > Think the latter two of these make more sense as DCHECKs in StartInternal (Know > you're copying from OnWebSocketHandshakeStreamReadyCallback, but think that gets > things wrong there, too) Hmm.. A few unit tests fail those two DCHECKs if I move them to StartInternal. https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:597: } else if (for_bidirectional_) { On 2015/10/08 19:31:53, mmenke wrote: > Hrm...Should we fail earlier for non-HTTPS/HTTP2 requests? I added a check in the wrapper to fail early for non-HTTPS requests. Is there a way to tell from the response info that this is a HTTP2 request? https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:1185: int HttpStreamFactoryImpl::Job::SetSpdyHttpStream( On 2015/10/08 19:31:53, mmenke wrote: > This method needs to be renamed. Done. https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/http_stream_f... net/http/http_stream_factory_impl_request.cc:310: } else if (for_bidirectional_) { On 2015/10/08 19:31:53, mmenke wrote: > Maybe add DCHECK(bidirectional_stream); DCHECK(!stream); > > And the opposite in the other branch? Done. https://codereview.chromium.org/1326503003/diff/180001/net/spdy/bidirectional... File net/spdy/bidirectional_spdy_stream.cc (right): https://codereview.chromium.org/1326503003/diff/180001/net/spdy/bidirectional... net/spdy/bidirectional_spdy_stream.cc:116: // Complete remaining buffered read. On 2015/10/07 23:44:56, mef wrote: > what if there is no pending read? Done. Good catch! I added null check to handle this case.
https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:43: virtual void OnDataSent() = 0; On 2015/10/19 21:07:46, xunjieli wrote: > On 2015/10/07 18:43:33, mmenke wrote: > > This is called when all data from the last SendData call is sent? > > I think it is called for every SendData. Right...But it's called when *all* of the data is sent. Note that this isn't how low level socket methods work, so I think it's worth noting that we send it when everything is sent, not just some subset of the data.
https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/180001/net/http/bidirectional... net/http/bidirectional_stream.h:43: virtual void OnDataSent() = 0; On 2015/10/19 21:27:53, mmenke wrote: > On 2015/10/19 21:07:46, xunjieli wrote: > > On 2015/10/07 18:43:33, mmenke wrote: > > > This is called when all data from the last SendData call is sent? > > > > I think it is called for every SendData. > > Right...But it's called when *all* of the data is sent. Note that this isn't > how low level socket methods work, so I think it's worth noting that we send it > when everything is sent, not just some subset of the data. Ah, I see! I will modify the comment to reflect this in the next patch set.
https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:50: virtual void OnClose(int status) = 0; Will there be distinction between OnClose and OnCancel? https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:72: // Sends data. This should not be called again util OnDataSent is invoked. until. Is OnDataSent invoked when entire buffer is sent? https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... File net/http/bidirectional_stream_create_helper.h (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:25: class NET_EXPORT BidirectionalStreamCreateHelper Could use a comment on class level. Does it have to be NET_EXPORT? Is it used from outside of net? https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:33: virtual void OnStreamFailed(int error) = 0; Is it net error? Could use a comment. https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... net/http/http_stream_factory.h:18: #include "net/http/bidirectional_stream.h" do you need the #include here, or can it be just forward-declared? https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:93: net_log, true /** for_bidirectional */); Can we define some enum for stream type like 'STREAM_TYPE_BIDIRECTIONAL' vs 'STREAM_TYPE_HTTP'? https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:409: /** bidirectional_stream=*/nullptr, I wonder what would happen if you call Pass() on scoped_ptr that holds null? https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:599: if (bidirectional_stream_ == nullptr) { why do you need this guard? Shouldn't it go to default: section for errors? https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:694: spurious nl? https://codereview.chromium.org/1326503003/diff/220001/net/spdy/bidirectional... File net/spdy/bidirectional_spdy_stream.cc (right): https://codereview.chromium.org/1326503003/diff/220001/net/spdy/bidirectional... net/spdy/bidirectional_spdy_stream.cc:197: delegate_->OnClose(OK); should we change this to 'noMoreData' flag?
On 2015/10/20 21:56:35, mef wrote: > https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... > File net/http/bidirectional_stream.h (right): > > https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... > net/http/bidirectional_stream.h:50: virtual void OnClose(int status) = 0; > Will there be distinction between OnClose and OnCancel? > > https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... > net/http/bidirectional_stream.h:72: // Sends data. This should not be called > again util OnDataSent is invoked. > until. Is OnDataSent invoked when entire buffer is sent? > > https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... > File net/http/bidirectional_stream_create_helper.h (right): > > https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... > net/http/bidirectional_stream_create_helper.h:25: class NET_EXPORT > BidirectionalStreamCreateHelper > Could use a comment on class level. > Does it have to be NET_EXPORT? > Is it used from outside of net? > > https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... > net/http/bidirectional_stream_create_helper.h:33: virtual void > OnStreamFailed(int error) = 0; > Is it net error? Could use a comment. > > https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... > File net/http/http_stream_factory.h (right): > > https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... > net/http/http_stream_factory.h:18: #include "net/http/bidirectional_stream.h" > do you need the #include here, or can it be just forward-declared? > > https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... > File net/http/http_stream_factory_impl.cc (right): > > https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... > net/http/http_stream_factory_impl.cc:93: net_log, true /** for_bidirectional > */); > Can we define some enum for stream type like 'STREAM_TYPE_BIDIRECTIONAL' vs > 'STREAM_TYPE_HTTP'? > > https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... > File net/http/http_stream_factory_impl_job.cc (right): > > https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... > net/http/http_stream_factory_impl_job.cc:409: /** > bidirectional_stream=*/nullptr, > I wonder what would happen if you call Pass() on scoped_ptr that holds null? > > https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... > net/http/http_stream_factory_impl_job.cc:599: if (bidirectional_stream_ == > nullptr) { > why do you need this guard? Shouldn't it go to default: section for errors? > > https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... > net/http/http_stream_factory_impl_job.cc:694: > spurious nl? > > https://codereview.chromium.org/1326503003/diff/220001/net/spdy/bidirectional... > File net/spdy/bidirectional_spdy_stream.cc (right): > > https://codereview.chromium.org/1326503003/diff/220001/net/spdy/bidirectional... > net/spdy/bidirectional_spdy_stream.cc:197: delegate_->OnClose(OK); > should we change this to 'noMoreData' flag? Thanks for the feedback! I think my understanding how the SpdyStream works might be incorrect. I will need some time to think this through and modify my tests. Will let you know once this is ready for review.
A few quick comments. https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:21: class NET_EXPORT BidirectionalStream { Need some class level docs. https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:60: virtual ~BidirectionalStream() {} Don't think we should inline anything in this file. https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:76: // canceled. I'd recommend against this behavior - we've run into issues with it in URLRequest, and want to get rid of it there. This is a single-threaded API, and we can cancel synchronously, so the embedder should be able to deterministicly determine the state of a BidirectionalStream. https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:80: private: nit: Blank line before private.
Thanks for the reviews! I modified the tests (Added one for the cancel case, and another one for small read buffer). PTAL. https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:21: class NET_EXPORT BidirectionalStream { On 2015/10/21 18:05:52, mmenke wrote: > Need some class level docs. Done. https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:50: virtual void OnClose(int status) = 0; On 2015/10/20 21:56:35, mef wrote: > Will there be distinction between OnClose and OnCancel? Not in the underlying implementation. But I think the caller of this interface can track whether Cancel is called? https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:60: virtual ~BidirectionalStream() {} On 2015/10/21 18:05:52, mmenke wrote: > Don't think we should inline anything in this file. What do you mean by inlining? Sorry, I am still a C++ newbie. Not sure what I should do here. https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:72: // Sends data. This should not be called again util OnDataSent is invoked. On 2015/10/20 21:56:34, mef wrote: > until. Is OnDataSent invoked when entire buffer is sent? Done. Yes, OnDataSent is invoked when the entire buffer in SendData is sent. Clarified in the comment. https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:76: // canceled. On 2015/10/21 18:05:52, mmenke wrote: > I'd recommend against this behavior - we've run into issues with it in > URLRequest, and want to get rid of it there. > > This is a single-threaded API, and we can cancel synchronously, so the embedder > should be able to deterministicly determine the state of a BidirectionalStream. Done. Good idea! That makes it easier to reason. https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:80: private: On 2015/10/21 18:05:52, mmenke wrote: > nit: Blank line before private. Done. https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... File net/http/bidirectional_stream_create_helper.h (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:25: class NET_EXPORT BidirectionalStreamCreateHelper On 2015/10/20 21:56:35, mef wrote: > Could use a comment on class level. Done. > Does it have to be NET_EXPORT? > Is it used from outside of net? It's going to be used by Cronet which is in components/, so it needs a NET_EXPORT, right? https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:33: virtual void OnStreamFailed(int error) = 0; On 2015/10/20 21:56:35, mef wrote: > Is it net error? Could use a comment. Done. https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... net/http/http_stream_factory.h:18: #include "net/http/bidirectional_stream.h" On 2015/10/20 21:56:35, mef wrote: > do you need the #include here, or can it be just forward-declared? Done. https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:93: net_log, true /** for_bidirectional */); On 2015/10/20 21:56:35, mef wrote: > Can we define some enum for stream type like 'STREAM_TYPE_BIDIRECTIONAL' vs > 'STREAM_TYPE_HTTP'? Done. Good idea! https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:409: /** bidirectional_stream=*/nullptr, On 2015/10/20 21:56:35, mef wrote: > I wonder what would happen if you call Pass() on scoped_ptr that holds null? I am not sure. I guess that will give nulls? I will add DCHECKs here. https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:599: if (bidirectional_stream_ == nullptr) { On 2015/10/20 21:56:35, mef wrote: > why do you need this guard? Shouldn't it go to default: section for errors? If H2 is not negotiated, we will still enter the OK case, so we need an extra check here to make sure we fail if H2 is not negotiated and bidi stream is requested. https://codereview.chromium.org/1326503003/diff/220001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:694: On 2015/10/20 21:56:35, mef wrote: > spurious nl? Done. https://codereview.chromium.org/1326503003/diff/220001/net/spdy/bidirectional... File net/spdy/bidirectional_spdy_stream.cc (right): https://codereview.chromium.org/1326503003/diff/220001/net/spdy/bidirectional... net/spdy/bidirectional_spdy_stream.cc:197: delegate_->OnClose(OK); On 2015/10/20 21:56:35, mef wrote: > should we change this to 'noMoreData' flag? Talked on the Misha's CL. I misunderstood the comment on OnDataReceived. We probably can use bytes_read = 0 as the signal for no more data (like how URLRequest does it). We will use OnClose as the signal for END_STREAM.
https://codereview.chromium.org/1326503003/diff/240001/net/http/bidirectional... File net/http/bidirectional_stream_create_helper.h (right): https://codereview.chromium.org/1326503003/diff/240001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:28: * A helper class to create a net::BidirectionalStream. Embedder should use Oops. I used Javadoc syntax. Will change in the next patch set.
https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:60: virtual ~BidirectionalStream() {} On 2015/10/21 19:35:36, xunjieli wrote: > On 2015/10/21 18:05:52, mmenke wrote: > > Don't think we should inline anything in this file. > > What do you mean by inlining? Sorry, I am still a C++ newbie. Not sure what I > should do here. Put method bodies with the declaration. You should move the bodies of both these methods to the cc file. Even "empty" methods can have some overhead when repeatedly compiled for every file that includes this header. Same goes fro Delegate() and ~Delegate().
https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/220001/net/http/bidirectional... net/http/bidirectional_stream.h:60: virtual ~BidirectionalStream() {} On 2015/10/21 19:46:14, mmenke wrote: > On 2015/10/21 19:35:36, xunjieli wrote: > > On 2015/10/21 18:05:52, mmenke wrote: > > > Don't think we should inline anything in this file. > > > > What do you mean by inlining? Sorry, I am still a C++ newbie. Not sure what I > > should do here. > > Put method bodies with the declaration. You should move the bodies of both > these methods to the cc file. Even "empty" methods can have some overhead when > repeatedly compiled for every file that includes this header. Same goes fro > Delegate() and ~Delegate(). I see! Done. Thanks https://codereview.chromium.org/1326503003/diff/240001/net/http/bidirectional... File net/http/bidirectional_stream_create_helper.h (right): https://codereview.chromium.org/1326503003/diff/240001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:28: * A helper class to create a net::BidirectionalStream. Embedder should use On 2015/10/21 19:38:21, xunjieli wrote: > Oops. I used Javadoc syntax. Will change in the next patch set. Done.
https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... File net/http/bidirectional_stream_create_helper.cc (right): https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:28: NetLog::SOURCE_BIDIRECTIONAL_STREAM)) { Hrm...this log is going to look a little weird. Normally you have a start and end event wrapping everything else, but since we don't have one object around for the life of the stream to log them to, that's not going to happen here. Weirdness around the BoundNetLog is what made me realize we needed a wrapper class in the first place. Looks like we're still going to have that weirdness, though. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:43: delegate_->OnStreamFailed(ERR_DISALLOWED_URL_SCHEME); Do we want to invoke callbacks synchronously, or guarantee that we call them all asynchronously? https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:64: delegate_->OnStreamCreated(); Should we pass in the stream here (wrapped in a scoped_ptr)? https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:78: DCHECK_NE(OK, result); If we want to DCHECK the result, think we really want: DCHECK_LT(result, 0); DCHECK_NE(result, ERR_IO_PENDING); Same goes for OnCertificateError. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:79: DCHECK(stream_request_.get()); nit: .get() isn't needed in any of these. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:121: NOTREACHED(); nit: include base/logging.h https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... File net/http/bidirectional_stream_create_helper.h (right): https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:28: // the Delegate interface to get notified of success or failure. Should mention that both the BidirectionalStreamCreateHelper and any streams it returns must be torn down before the URLRequestContext (HttpNetworkSession? See below comment) https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:32: class NET_EXPORT Delegate { Should document this - in particular, are callbacks guaranteed to be called asynchronously? https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:47: BidirectionalStreamCreateHelper(const HttpRequestInfo* request_info, const HttpRequestInfo& request_info? https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:47: BidirectionalStreamCreateHelper(const HttpRequestInfo* request_info, Worth mentioning that parts of request_info are ignored (upload_data_stream and most load flags? Could even DCHECK that the upload data stream is NULL, and most load flags aren't set). https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:49: const URLRequestContext* context, Maybe just take an HTTP network session? That allows not having to create an entire network stack to use it - not that I see anyone doing that, but we don't require anything above the stream factory. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:49: const URLRequestContext* context, "const" is kinda weird here - by making a request, we do basically modify the context. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:52: ~BidirectionalStreamCreateHelper() override; Should mention somewhere that destroying it cancels any pending request. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:55: void CreateBidirectionalStream(); Should we do this automatically on creation? Saves us from having to worry about this method being called twice.
Patchset #8 (id:280001) has been deleted
Thanks for the review! PTAL https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... File net/http/bidirectional_stream_create_helper.cc (right): https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:28: NetLog::SOURCE_BIDIRECTIONAL_STREAM)) { On 2015/10/21 22:40:16, mmenke wrote: > Hrm...this log is going to look a little weird. Normally you have a start and > end event wrapping everything else, but since we don't have one object around > for the life of the stream to log them to, that's not going to happen here. > > Weirdness around the BoundNetLog is what made me realize we needed a wrapper > class in the first place. Looks like we're still going to have that weirdness, > though. Should we make the embedder (who presumably will own the stream for its lifetime) pass us a BoundNetLog (which it owns too) through the CreateHelper? Not sure if that will prevent the weirdness. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:43: delegate_->OnStreamFailed(ERR_DISALLOWED_URL_SCHEME); On 2015/10/21 22:40:16, mmenke wrote: > Do we want to invoke callbacks synchronously, or guarantee that we call them all > asynchronously? Done. Probably the latter to be more consistent? https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:64: delegate_->OnStreamCreated(); On 2015/10/21 22:40:16, mmenke wrote: > Should we pass in the stream here (wrapped in a scoped_ptr)? Done. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:78: DCHECK_NE(OK, result); On 2015/10/21 22:40:16, mmenke wrote: > If we want to DCHECK the result, think we really want: > > DCHECK_LT(result, 0); > DCHECK_NE(result, ERR_IO_PENDING); > > Same goes for OnCertificateError. Done. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:79: DCHECK(stream_request_.get()); On 2015/10/21 22:40:16, mmenke wrote: > nit: .get() isn't needed in any of these. Done. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:121: NOTREACHED(); On 2015/10/21 22:40:16, mmenke wrote: > nit: include base/logging.h Done. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... File net/http/bidirectional_stream_create_helper.h (right): https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:28: // the Delegate interface to get notified of success or failure. On 2015/10/21 22:40:16, mmenke wrote: > Should mention that both the BidirectionalStreamCreateHelper and any streams it > returns must be torn down before the URLRequestContext (HttpNetworkSession? See > below comment) Done. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:32: class NET_EXPORT Delegate { On 2015/10/21 22:40:16, mmenke wrote: > Should document this - in particular, are callbacks guaranteed to be called > asynchronously? Done. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:47: BidirectionalStreamCreateHelper(const HttpRequestInfo* request_info, On 2015/10/21 22:40:16, mmenke wrote: > const HttpRequestInfo& request_info? Done. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:47: BidirectionalStreamCreateHelper(const HttpRequestInfo* request_info, On 2015/10/21 22:40:16, mmenke wrote: > Worth mentioning that parts of request_info are ignored (upload_data_stream and > most load flags? Could even DCHECK that the upload data stream is NULL, and > most load flags aren't set). Done. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:49: const URLRequestContext* context, On 2015/10/21 22:40:16, mmenke wrote: > "const" is kinda weird here - by making a request, we do basically modify the > context. Done. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:49: const URLRequestContext* context, On 2015/10/21 22:40:16, mmenke wrote: > Maybe just take an HTTP network session? That allows not having to create an > entire network stack to use it - not that I see anyone doing that, but we don't > require anything above the stream factory. Done. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:52: ~BidirectionalStreamCreateHelper() override; On 2015/10/21 22:40:16, mmenke wrote: > Should mention somewhere that destroying it cancels any pending request. Done. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.h:55: void CreateBidirectionalStream(); On 2015/10/21 22:40:16, mmenke wrote: > Should we do this automatically on creation? Saves us from having to worry > about this method being called twice. Done. Good idea
https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... File net/http/bidirectional_stream_create_helper.cc (right): https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:28: NetLog::SOURCE_BIDIRECTIONAL_STREAM)) { On 2015/10/22 20:01:41, xunjieli wrote: > On 2015/10/21 22:40:16, mmenke wrote: > > Hrm...this log is going to look a little weird. Normally you have a start and > > end event wrapping everything else, but since we don't have one object around > > for the life of the stream to log them to, that's not going to happen here. > > > > Weirdness around the BoundNetLog is what made me realize we needed a wrapper > > class in the first place. Looks like we're still going to have that > weirdness, > > though. > > Should we make the embedder (who presumably will own the stream for its > lifetime) pass us a BoundNetLog (which it owns too) through the CreateHelper? > Not sure if that will prevent the weirdness. That wouldn't - the way things elsewhere is that there's a class that represents the same object as the BoundNetLog, and it logs a start and end event. I don't think we want to make our embedder do that logging. Here, there's no class that is around for the duration of the log. Only way I see to get that would be to make this stream take ownership of the created BidirectionalStream, and then all calls to the stream to go through this. Then this could just log an entry on creation and destruction, and we'd done. https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:43: delegate_->OnStreamFailed(ERR_DISALLOWED_URL_SCHEME); On 2015/10/22 20:01:41, xunjieli wrote: > On 2015/10/21 22:40:16, mmenke wrote: > > Do we want to invoke callbacks synchronously, or guarantee that we call them > all > > asynchronously? > > Done. Probably the latter to be more consistent? I think async is indeed the way to go - avoids re-entrancy issues (And is basically required if BidirectionalStreamCreateHelper creation automatically starts creating a stream, too).
Patchset #9 (id:320001) has been deleted
https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... File net/http/bidirectional_stream_create_helper.cc (right): https://codereview.chromium.org/1326503003/diff/260001/net/http/bidirectional... net/http/bidirectional_stream_create_helper.cc:28: NetLog::SOURCE_BIDIRECTIONAL_STREAM)) { On 2015/10/22 20:13:36, mmenke wrote: > On 2015/10/22 20:01:41, xunjieli wrote: > > On 2015/10/21 22:40:16, mmenke wrote: > > > Hrm...this log is going to look a little weird. Normally you have a start > and > > > end event wrapping everything else, but since we don't have one object > around > > > for the life of the stream to log them to, that's not going to happen here. > > > > > > Weirdness around the BoundNetLog is what made me realize we needed a wrapper > > > class in the first place. Looks like we're still going to have that > > weirdness, > > > though. > > > > Should we make the embedder (who presumably will own the stream for its > > lifetime) pass us a BoundNetLog (which it owns too) through the CreateHelper? > > Not sure if that will prevent the weirdness. > > That wouldn't - the way things elsewhere is that there's a class that represents > the same object as the BoundNetLog, and it logs a start and end event. I don't > think we want to make our embedder do that logging. > > Here, there's no class that is around for the duration of the log. Only way I > see to get that would be to make this stream take ownership of the created > BidirectionalStream, and then all calls to the stream to go through this. Then > this could just log an entry on creation and destruction, and we'd done. Done. Thanks for the detailed explanation! I think I understand the use of BoundNetLog a little better. I have changed this wrapper class to own the stream, and added proxy methods to manipulate the stream through this wrapper class. Let me know what you think.
A friendly ping..
https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:39: virtual void OnHeaders(const SpdyHeaderBlock& response_headers) = 0; Is this called only once or could it happen multiple times? https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:81: // TODO(xunjieli): implement a method to do flow control. We also need a method for ping, but fine to have it in another CL. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream_helper.h (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper.h:57: // Destroys the helper also cancels any pending request: nit: also -> and https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream_helper_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper_unittest.cc:53: TEST_F(BidirectionalStreamHelperTest, CreateInsecureStream) { Do we need a successful test as well?
Some quick comments - sorry I haven't been more active on this one. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:24: class NET_EXPORT BidirectionalStream { NET_EXPORT_PRIVATE (You'll need to make the helper class implement this class's delegate, and make the helper's delegate match this one - the wrapper is a bit of a hassle, but it makes lifetime semantics a little clearer, cancellation cleaner, and works a bit more like URLRequests). https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream_helper.cc (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper.cc:53: void BidirectionalStreamHelper::Start(BidirectionalStream::Delegate* delegate) { Destroy the stream_request? Not so concerned about keeping it around in the other cases. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper.cc:73: void BidirectionalStreamHelper::Cancel() { If we're still requesting a stream, should cancel the stream request. Should also update documentation. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream_helper.h (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper.h:27: // BidirectionalStreamHelper must be torn down before the HttpNetworkSession. Should probably make this the only externally visible class here - move consumer documentation to this class, and maybe rename it to make it sound more like a class people should be using. Not really sure on the naming. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream_helper_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper_unittest.cc:46: scoped_ptr<base::RunLoop> loop_; don't need to make this a scoped_ptr. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper_unittest.cc:49: } } // namespace https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper_unittest.cc:49: } blank line before end of namespace. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper_unittest.cc:51: class BidirectionalStreamHelperTest : public testing::Test {}; Don't need this - just use TEST instead of TEST_F.
Few questions that I got while trying to use it from Cronet side. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:33: virtual void OnFailed(int error) = 0; UrlRequest has a status() method. Does this callback replace it? https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:36: virtual void OnRequestHeadersSent() = 0; We need to add something like HttpResponseInfo, with details about negotiated protocol, etc. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:39: virtual void OnHeaders(const SpdyHeaderBlock& response_headers) = 0; Is there some way to get Http status code at this point? https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:43: virtual void OnReadCompleted(int bytes_read) = 0; Is there some way to get running received bytes count (pre-decompression)?
https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:33: virtual void OnFailed(int error) = 0; On 2015/11/04 20:13:53, mef wrote: > UrlRequest has a status() method. Does this callback replace it? This and knowledge of whether you're waiting on it to call you or not. I'd love to be able to remove status() from UrlRequest - it makes for a weird API, and lots of fun bugs in both the network stack and consumers of it.
Patchset #10 (id:360001) has been deleted
Patchset #10 (id:380001) has been deleted
Patchset #10 (id:400001) has been deleted
Patchset #10 (id:420001) has been deleted
Thanks for the review! I renamed the wrapper class to BidirectionalStream, and the interface shared between spdy and quic as BidirectionalStreamJob. This is slightly closer to what we have for URLRequest and URLRequestJob. Let me know what you think. PTAL. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:24: class NET_EXPORT BidirectionalStream { On 2015/11/03 20:32:13, mmenke wrote: > NET_EXPORT_PRIVATE (You'll need to make the helper class implement this class's > delegate, and make the helper's delegate match this one - the wrapper is a bit > of a hassle, but it makes lifetime semantics a little clearer, cancellation > cleaner, and works a bit more like URLRequests). Done. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:36: virtual void OnRequestHeadersSent() = 0; On 2015/11/04 20:13:53, mef wrote: > We need to add something like HttpResponseInfo, with details about negotiated > protocol, etc. Acknowledged. Added as a TODO. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:39: virtual void OnHeaders(const SpdyHeaderBlock& response_headers) = 0; On 2015/11/02 18:00:42, mef wrote: > Is this called only once or could it happen multiple times? It's called once. I've added comment. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:39: virtual void OnHeaders(const SpdyHeaderBlock& response_headers) = 0; On 2015/11/04 20:13:53, mef wrote: > Is there some way to get Http status code at this point? Yes, we can get it from the header ":status" https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:43: virtual void OnReadCompleted(int bytes_read) = 0; On 2015/11/04 20:13:52, mef wrote: > Is there some way to get running received bytes count (pre-decompression)? This byte count here is pre-decompression. We can increment the counter cronet side. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream.h:81: // TODO(xunjieli): implement a method to do flow control. On 2015/11/02 18:00:42, mef wrote: > We also need a method for ping, but fine to have it in another CL. Acknowledged. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream_helper.cc (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper.cc:53: void BidirectionalStreamHelper::Start(BidirectionalStream::Delegate* delegate) { On 2015/11/03 20:32:13, mmenke wrote: > Destroy the stream_request? Not so concerned about keeping it around in the > other cases. Done. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper.cc:73: void BidirectionalStreamHelper::Cancel() { On 2015/11/03 20:32:13, mmenke wrote: > If we're still requesting a stream, should cancel the stream request. Should > also update documentation. Done. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream_helper.h (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper.h:27: // BidirectionalStreamHelper must be torn down before the HttpNetworkSession. On 2015/11/03 20:32:13, mmenke wrote: > Should probably make this the only externally visible class here - move consumer > documentation to this class, and maybe rename it to make it sound more like a > class people should be using. Not really sure on the naming. Done. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper.h:57: // Destroys the helper also cancels any pending request: On 2015/11/02 18:00:42, mef wrote: > nit: also -> and Done. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... File net/http/bidirectional_stream_helper_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper_unittest.cc:46: scoped_ptr<base::RunLoop> loop_; On 2015/11/03 20:32:13, mmenke wrote: > don't need to make this a scoped_ptr. Done. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper_unittest.cc:49: } On 2015/11/03 20:32:13, mmenke wrote: > } // namespace Done. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper_unittest.cc:49: } On 2015/11/03 20:32:13, mmenke wrote: > blank line before end of namespace. Done. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper_unittest.cc:51: class BidirectionalStreamHelperTest : public testing::Test {}; On 2015/11/03 20:32:13, mmenke wrote: > Don't need this - just use TEST instead of TEST_F. Done. https://codereview.chromium.org/1326503003/diff/340001/net/http/bidirectional... net/http/bidirectional_stream_helper_unittest.cc:53: TEST_F(BidirectionalStreamHelperTest, CreateInsecureStream) { On 2015/11/02 18:00:42, mef wrote: > Do we need a successful test as well? Done.
Patchset #11 (id:460001) has been deleted
Could you sync it to HEAD? I've tried to patch it and failed.
On 2015/11/06 18:03:02, mef wrote: > Could you sync it to HEAD? I've tried to patch it and failed. Done. I didn't see any rebase errors though. Could you try again?
On 2015/11/06 18:25:58, xunjieli wrote: > On 2015/11/06 18:03:02, mef wrote: > > Could you sync it to HEAD? I've tried to patch it and failed. > > Done. I didn't see any rebase errors though. Could you try again? Thanks, the rebase worked! So, how do I start the stream job? Is there an example that I can follow?
On 2015/11/09 21:01:59, mef wrote: > On 2015/11/06 18:25:58, xunjieli wrote: > > On 2015/11/06 18:03:02, mef wrote: > > > Could you sync it to HEAD? I've tried to patch it and failed. > > > > Done. I didn't see any rebase errors though. Could you try again? > > Thanks, the rebase worked! So, how do I start the stream job? > Is there an example that I can follow? Cronet's adapter can implement BidirectionalStream::Delegate, create and own a BidirectionalStream. Creating BidirectionalStream implicitly starts the stream job and sends the headers. An example is in net/http/bidirectional_stream_unittest.cc, where the test calls BidirectionalStream's methods to write data once headers is sent (Delegate::OnRequestHeadersSent is called), and starts reading when response headers are received. Let me know if this makes sense.
On 2015/11/09 21:14:16, xunjieli wrote: > On 2015/11/09 21:01:59, mef wrote: > > On 2015/11/06 18:25:58, xunjieli wrote: > > > On 2015/11/06 18:03:02, mef wrote: > > > > Could you sync it to HEAD? I've tried to patch it and failed. > > > > > > Done. I didn't see any rebase errors though. Could you try again? > > > > Thanks, the rebase worked! So, how do I start the stream job? > > Is there an example that I can follow? > > Cronet's adapter can implement BidirectionalStream::Delegate, create and own a > BidirectionalStream. Creating BidirectionalStream implicitly starts the stream > job and sends the headers. An example is in > net/http/bidirectional_stream_unittest.cc, where the test calls > BidirectionalStream's methods to write data once headers is sent > (Delegate::OnRequestHeadersSent is called), and starts reading when response > headers are received. Let me know if this makes sense. That seems to work, the HttpStreamFactoryImpl::Job gets created, but net::HttpStreamFactoryImpl::Request::OnStreamFailed gets ERR_FAILED: Stack frame #06 pc 00047827 /data/app/org.chromium.net-1/lib/arm/libcronet_tests.so: Routine logging::LogMessage::~LogMessage() at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/logging.cc:601 Stack frame #07 pc 0002423b /data/app/org.chromium.net-1/lib/arm/libcronet_tests.so: Routine cronet::CronetBidirectionalStream::OnFailed(net::Error) at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../components/cronet/android/cronet_bidirectional_stream.cc:251 (discriminator 5) Stack frame #08 pc 00114047 /data/app/org.chromium.net-1/lib/arm/libcronet_tests.so: Routine net::BidirectionalStream::OnStreamFailed(int, net::SSLConfig const&, net::SSLFailureState) at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../net/http/bidirectional_stream.cc:145 Stack frame #09 pc 00137d51 /data/app/org.chromium.net-1/lib/arm/libcronet_tests.so: Routine net::HttpStreamFactoryImpl::Request::OnStreamFailed(net::HttpStreamFactoryImpl::Job*, int, net::SSLConfig const&, net::SSLFailureState) at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../net/http/http_stream_factory_impl_request.cc:145 Stack frame #10 pc 00133fa3 /data/app/org.chromium.net-1/lib/arm/libcronet_tests.so: Routine net::HttpStreamFactoryImpl::Job::OnStreamFailedCallback(int) at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../net/http/http_stream_factory_impl_job.cc:430 Stack frame #11 pc 001340c1 /data/app/org.chromium.net-1/lib/arm/libcronet_tests.so: Routine base::internal::InvokeHelper<true, void, base::internal::RunnableAdapter<void (net::HttpStreamFactoryImpl::Job::*)(int)>, base::internal::TypeList<base::WeakPtr<net::HttpStreamFactoryImpl::Job> const&, net::Error const&> >::MakeItSo(base::internal::RunnableAdapter<void (net::HttpStreamFactoryImpl::Job::*)(int)>, base::WeakPtr<net::HttpStreamFactoryImpl::Job> const&, net::Error const&) at /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/bind_internal.h:303 (discriminator 2)
On 2015/11/09 22:44:47, mef wrote: > On 2015/11/09 21:14:16, xunjieli wrote: > > On 2015/11/09 21:01:59, mef wrote: > > > On 2015/11/06 18:25:58, xunjieli wrote: > > > > On 2015/11/06 18:03:02, mef wrote: > > > > > Could you sync it to HEAD? I've tried to patch it and failed. > > > > > > > > Done. I didn't see any rebase errors though. Could you try again? > > > > > > Thanks, the rebase worked! So, how do I start the stream job? > > > Is there an example that I can follow? > > > > Cronet's adapter can implement BidirectionalStream::Delegate, create and own a > > BidirectionalStream. Creating BidirectionalStream implicitly starts the stream > > job and sends the headers. An example is in > > net/http/bidirectional_stream_unittest.cc, where the test calls > > BidirectionalStream's methods to write data once headers is sent > > (Delegate::OnRequestHeadersSent is called), and starts reading when response > > headers are received. Let me know if this makes sense. > > That seems to work, the HttpStreamFactoryImpl::Job gets created, but > net::HttpStreamFactoryImpl::Request::OnStreamFailed gets ERR_FAILED: > > Stack frame #06 pc 00047827 > /data/app/org.chromium.net-1/lib/arm/libcronet_tests.so: Routine > logging::LogMessage::~LogMessage() at > /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/logging.cc:601 > Stack frame #07 pc 0002423b > /data/app/org.chromium.net-1/lib/arm/libcronet_tests.so: Routine > cronet::CronetBidirectionalStream::OnFailed(net::Error) at > /usr/local/google/home/mef/chrome/android/src/out/Debug/../../components/cronet/android/cronet_bidirectional_stream.cc:251 > (discriminator 5) > Stack frame #08 pc 00114047 > /data/app/org.chromium.net-1/lib/arm/libcronet_tests.so: Routine > net::BidirectionalStream::OnStreamFailed(int, net::SSLConfig const&, > net::SSLFailureState) at > /usr/local/google/home/mef/chrome/android/src/out/Debug/../../net/http/bidirectional_stream.cc:145 > Stack frame #09 pc 00137d51 > /data/app/org.chromium.net-1/lib/arm/libcronet_tests.so: Routine > net::HttpStreamFactoryImpl::Request::OnStreamFailed(net::HttpStreamFactoryImpl::Job*, > int, net::SSLConfig const&, net::SSLFailureState) at > /usr/local/google/home/mef/chrome/android/src/out/Debug/../../net/http/http_stream_factory_impl_request.cc:145 > Stack frame #10 pc 00133fa3 > /data/app/org.chromium.net-1/lib/arm/libcronet_tests.so: Routine > net::HttpStreamFactoryImpl::Job::OnStreamFailedCallback(int) at > /usr/local/google/home/mef/chrome/android/src/out/Debug/../../net/http/http_stream_factory_impl_job.cc:430 > Stack frame #11 pc 001340c1 > /data/app/org.chromium.net-1/lib/arm/libcronet_tests.so: Routine > base::internal::InvokeHelper<true, void, base::internal::RunnableAdapter<void > (net::HttpStreamFactoryImpl::Job::*)(int)>, > base::internal::TypeList<base::WeakPtr<net::HttpStreamFactoryImpl::Job> const&, > net::Error const&> >::MakeItSo(base::internal::RunnableAdapter<void > (net::HttpStreamFactoryImpl::Job::*)(int)>, > base::WeakPtr<net::HttpStreamFactoryImpl::Job> const&, net::Error const&) at > /usr/local/google/home/mef/chrome/android/src/out/Debug/../../base/bind_internal.h:303 > (discriminator 2) Misha found that when session isn't already established, the stream will fail. I will look into this tomorrow.
https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream.h:61: // called after this. |status| is an error code or OK. Is |status| indeed a net Error Code? https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream.h:72: // load flags). That's a good point, should we specify which load_flags are not ignored? https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream.h:82: // or ERR_IO_PENDING if the read is to be completed asynchronously. Can this have more than one call in flight? If not maybe add a comment similar to one on SendData. https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream.h:90: void Cancel(); What if BidirectionalStream is deleted without being canceled? https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream.h:148: scoped_ptr<BidirectionalStreamJob> stream_; should this be called |stream_job_| to avoid confusion with |this| being a stream? https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... File net/http/bidirectional_stream_job.h (right): https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream_job.h:21: // An interface that exposes bidirectional streaming. Maybe add a comment about lifetime scenarios that are addressed by having BidirectionalStreamJob?
Patchset #12 (id:500001) has been deleted
Thanks for the review! Misha, I fixed the session establish bug that you observed earlier. The cause is that I didn't copy out the ALPN and NPN protos into SSLConfig. It should work now. Could you try again? https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream.h:61: // called after this. |status| is an error code or OK. On 2015/11/10 23:00:27, mef wrote: > Is |status| indeed a net Error Code? Done. Yes. I modified the comment to make this clear. https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream.h:72: // load flags). On 2015/11/10 23:00:27, mef wrote: > That's a good point, should we specify which load_flags are not ignored? I'd like to say all for now. But I don't really know.. We currently have a check to make sure there aren't any load flags set. (DCHECK_EQ(LOAD_NORMAL, request_info.load_flags) in bidirectional_stream.cc) https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream.h:82: // or ERR_IO_PENDING if the read is to be completed asynchronously. On 2015/11/10 23:00:27, mef wrote: > Can this have more than one call in flight? If not maybe add a comment similar > to one on SendData. Done. https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream.h:90: void Cancel(); On 2015/11/10 23:00:27, mef wrote: > What if BidirectionalStream is deleted without being canceled? Hmm.. Then we probably should call cancel in that case. Done. https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream.h:148: scoped_ptr<BidirectionalStreamJob> stream_; On 2015/11/10 23:00:27, mef wrote: > should this be called |stream_job_| to avoid confusion with |this| being a > stream? Done. https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... File net/http/bidirectional_stream_job.h (right): https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream_job.h:21: // An interface that exposes bidirectional streaming. On 2015/11/10 23:00:27, mef wrote: > Maybe add a comment about lifetime scenarios that are addressed by having > BidirectionalStreamJob? I am not very sure what the lifetime scenarios you have in mind. Could you elaborate a bit?
On 2015/11/13 23:41:45, xunjieli wrote: > Thanks for the review! Misha, I fixed the session establish bug that you > observed earlier. The cause is that I didn't copy out the ALPN and NPN protos > into SSLConfig. It should work now. Could you try again? Tried, and it works, thanks a lot! > https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... > File net/http/bidirectional_stream_job.h (right): > > https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... > net/http/bidirectional_stream_job.h:21: // An interface that exposes > bidirectional streaming. > On 2015/11/10 23:00:27, mef wrote: > > Maybe add a comment about lifetime scenarios that are addressed by having > > BidirectionalStreamJob? > > I am not very sure what the lifetime scenarios you have in mind. Could you > elaborate a bit? Um, something that would explain why do we need both BidirectionalStream and BidirectionalStreamJob.
https://codereview.chromium.org/1326503003/diff/520001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/520001/net/http/bidirectional... net/http/bidirectional_stream.h:61: // called after this. |status| is a net error code or OK. If |status| is a net error code, then how is it different than |OnFailed|? Can other callback methods still be called after OnFailed?
Patchset #14 (id:560001) has been deleted
Patchset #14 (id:580001) has been deleted
Thanks, PTAL. https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... File net/http/bidirectional_stream_job.h (right): https://codereview.chromium.org/1326503003/diff/480001/net/http/bidirectional... net/http/bidirectional_stream_job.h:21: // An interface that exposes bidirectional streaming. On 2015/11/13 23:41:45, xunjieli wrote: > On 2015/11/10 23:00:27, mef wrote: > > Maybe add a comment about lifetime scenarios that are addressed by having > > BidirectionalStreamJob? > > I am not very sure what the lifetime scenarios you have in mind. Could you > elaborate a bit? Done. https://codereview.chromium.org/1326503003/diff/520001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/520001/net/http/bidirectional... net/http/bidirectional_stream.h:61: // called after this. |status| is a net error code or OK. On 2015/11/24 20:03:40, mef wrote: > If |status| is a net error code, then how is it different than |OnFailed|? Can > other callback methods still be called after OnFailed? Done. Good point. I think we should combine these two.
I am planning to write a few more tests to make sure the edge cases are handled correctly. No hurry in reviewing.
A couple quick comments. https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... net/http/bidirectional_stream.h:44: virtual void OnHeaders(const SpdyHeaderBlock& response_headers) = 0; SpdyHeaderBlock? We're going to be wrapping QUIC, too, eventually, right? (Also, you're not forward declaring SpdyHeaderBlock) https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... net/http/bidirectional_stream.h:54: virtual void OnTrailers(const SpdyHeaderBlock& trailers) = 0; SpdyHeaderBlock? We're going to be wrapping QUIC, too, eventually, right? https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... net/http/bidirectional_stream.h:69: BidirectionalStream(const HttpRequestInfo& request_info, May be cleaner to take a struct with just what we support (No load flags, if we decide we need some, can take bools for those that we do, no privacy mode [I assume], etc). https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... net/http/bidirectional_stream.h:90: // TODO(xunjieli): implement a method to do flow control, and a method to ping Tiny grammar nit: Remove comma. https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... net/http/bidirectional_stream.h:106: void OnClose(int status) override; Remove blank lines between these methods (And the one after the "implementation" line) https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... net/http/bidirectional_stream.h:109: nit: Remove blank line. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:28: const base::StringPiece kBodyDataStringPiece(kBodyData, kBodyDataSize); Global variables must be POD types using initializer lists. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:31: const size_t kSmallReadBufferSize = 4; Should document kSmallReadBufferSize https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:33: class TestBidirectionalStreamSpdyJobDelegate Should document this class. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:89: int close_status_; Can we just make these private, and add const accessors? https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:93: scoped_ptr<base::RunLoop> loop_; Suggest making these private, too, and adding protected methods for what you want to do with them (close the stream, quit the runloop). Makes the API clearer. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:96: void StartOrContinueReading() { Should document this. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:107: std::string data_received_; These should all use DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:150: void OnDataSent() override { ASSERT_TRUE(false); } GTEST_FAIL() (Or just NOTREACHED()), for all of these. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:197: spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, LOWEST, true)); NULL -> nullptr, for all of these.
Patchset #15 (id:620001) has been deleted
Thanks for the review! I still need to think of a way to drive the SpdyStream::Delegate signals in a non-flaky way (since the code is time dependent). Although I copied the buffering mechanism from SpdyHttpStream, I think it is dangerous to leave it untested since our API is slightly different. We need to have the buffering in place because SpdyStream::Delegate::OnDataReceived is called for every H2 DATA frame, which might be too small a unit to propagate to caller. Happy holiday and I will get back to this after the break. https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... net/http/bidirectional_stream.h:44: virtual void OnHeaders(const SpdyHeaderBlock& response_headers) = 0; Yea. But it looks like QUIC is also using SpdyHeaderBlock too. https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_reli... https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... net/http/bidirectional_stream.h:54: virtual void OnTrailers(const SpdyHeaderBlock& trailers) = 0; On 2015/11/25 18:39:18, mmenke wrote: > SpdyHeaderBlock? We're going to be wrapping QUIC, too, eventually, right? Same as above. https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... net/http/bidirectional_stream.h:69: BidirectionalStream(const HttpRequestInfo& request_info, On 2015/11/25 18:39:18, mmenke wrote: > May be cleaner to take a struct with just what we support (No load flags, if we > decide we need some, can take bools for those that we do, no privacy mode [I > assume], etc). Done. https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... net/http/bidirectional_stream.h:90: // TODO(xunjieli): implement a method to do flow control, and a method to ping On 2015/11/25 18:39:18, mmenke wrote: > Tiny grammar nit: Remove comma. Done. https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... net/http/bidirectional_stream.h:106: void OnClose(int status) override; On 2015/11/25 18:39:18, mmenke wrote: > Remove blank lines between these methods (And the one after the "implementation" > line) Done. https://codereview.chromium.org/1326503003/diff/600001/net/http/bidirectional... net/http/bidirectional_stream.h:109: On 2015/11/25 18:39:18, mmenke wrote: > nit: Remove blank line. Done. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:28: const base::StringPiece kBodyDataStringPiece(kBodyData, kBodyDataSize); On 2015/11/25 18:39:18, mmenke wrote: > Global variables must be POD types using initializer lists. Done. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:31: const size_t kSmallReadBufferSize = 4; On 2015/11/25 18:39:18, mmenke wrote: > Should document kSmallReadBufferSize Done. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:33: class TestBidirectionalStreamSpdyJobDelegate On 2015/11/25 18:39:18, mmenke wrote: > Should document this class. Done. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:89: int close_status_; On 2015/11/25 18:39:18, mmenke wrote: > Can we just make these private, and add const accessors? Done. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:93: scoped_ptr<base::RunLoop> loop_; On 2015/11/25 18:39:18, mmenke wrote: > Suggest making these private, too, and adding protected methods for what you > want to do with them (close the stream, quit the runloop). Makes the API > clearer. Done. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:96: void StartOrContinueReading() { On 2015/11/25 18:39:18, mmenke wrote: > Should document this. Done. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:107: std::string data_received_; On 2015/11/25 18:39:18, mmenke wrote: > These should all use DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:150: void OnDataSent() override { ASSERT_TRUE(false); } On 2015/11/25 18:39:18, mmenke wrote: > GTEST_FAIL() (Or just NOTREACHED()), for all of these. Done. https://codereview.chromium.org/1326503003/diff/600001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job_unittest.cc:197: spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, LOWEST, true)); On 2015/11/25 18:39:18, mmenke wrote: > NULL -> nullptr, for all of these. Done.
Done. I have introduced a MockTimer and DeterministicSocketData to manually drive the SpdyStream::Delegate signals. I know that we are getting rid of DeterministicSocketData, but I didn't find a better alternative since we can't really mock out SpdyStream. I have added two tests to make sure the buffering mechanism is working as expected. Please feel free to suggest more. Thanks for the patience. PTAL.
Patchset #18 (id:700001) has been deleted
Misha pointed out another bug in the code. I made necessary edits and wrote a test case for it. While I was writing the test, I found that I was not delaying the OnClose signal. For example, if a user calls ReadData after SpdyStream::Delegate::OnClose is invoked, the ReadData call will return synchronously (since we have all data buffered and ready to serve), and BidirectionalStream::Delegate::OnClose will not ever be called! So I decided that delaying BidirectionalStream::Delegate::OnClose gives little benefit and is tricky to handle. Therefore, I changed BidirectionalStream::Delegate::OnClose to BidirectionalStream::Delegate::OnFailed which is only invoked upon failure. User of BidirectionalStream can know that there is nothing to read when ReadData return 0 (which indicates EOF), which should be enough. Misha, let me know what you think. I have uploaded a new patch (#18).
On 2015/12/02 02:20:12, xunjieli wrote: > Misha pointed out another bug in the code. I made necessary edits and wrote a > test case for it. While I was writing the test, I found that I was not delaying > the OnClose signal. For example, if a user calls ReadData after > SpdyStream::Delegate::OnClose is invoked, the ReadData call will return > synchronously (since we have all data buffered and ready to serve), and > BidirectionalStream::Delegate::OnClose will not ever be called! So I decided > that delaying BidirectionalStream::Delegate::OnClose gives little benefit and is > tricky to handle. Therefore, I changed BidirectionalStream::Delegate::OnClose to > BidirectionalStream::Delegate::OnFailed which is only invoked upon failure. User > of BidirectionalStream can know that there is nothing to read when ReadData > return 0 (which indicates EOF), which should be enough. > > Misha, let me know what you think. I have uploaded a new patch (#18). Hrm, does it mean that the app must always call 'Read' even there is no data to be received, for example with 'HEAD' verb?
On 2015/12/02 13:54:29, mef wrote: > On 2015/12/02 02:20:12, xunjieli wrote: > > Misha pointed out another bug in the code. I made necessary edits and wrote a > > test case for it. While I was writing the test, I found that I was not > delaying > > the OnClose signal. For example, if a user calls ReadData after > > SpdyStream::Delegate::OnClose is invoked, the ReadData call will return > > synchronously (since we have all data buffered and ready to serve), and > > BidirectionalStream::Delegate::OnClose will not ever be called! So I decided > > that delaying BidirectionalStream::Delegate::OnClose gives little benefit and > is > > tricky to handle. Therefore, I changed BidirectionalStream::Delegate::OnClose > to > > BidirectionalStream::Delegate::OnFailed which is only invoked upon failure. > User > > of BidirectionalStream can know that there is nothing to read when ReadData > > return 0 (which indicates EOF), which should be enough. > > > > Misha, let me know what you think. I have uploaded a new patch (#18). > > Hrm, does it mean that the app must always call 'Read' even there is no data to > be received, for example with 'HEAD' verb? I think we still need a signal when stream is closed by both sides. It is possible that stream is half-closed from the remote first, and from the local second, so getting onReadCompleted with 0 length doesn't seem like a sufficient indicator.
https://codereview.chromium.org/1326503003/diff/700002/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/700002/net/http/bidirectional... net/http/bidirectional_stream.h:58: // No other delegate functions will be called after this. I don't think this is correct statement. I think we've said that OnTrailers will be called when trailers frame arrives, not after completion of all reads and writes. https://codereview.chromium.org/1326503003/diff/700002/net/http/bidirectional... net/http/bidirectional_stream.h:63: virtual void OnFailed(int error) = 0; I think we need either OnClose, or OnSucceeded to indicate successful close of the stream. https://codereview.chromium.org/1326503003/diff/700002/net/net.gypi File net/net.gypi (right): https://codereview.chromium.org/1326503003/diff/700002/net/net.gypi#newcode1741 net/net.gypi:1741: 'tools/quic/test_tools/mock_quic_server_session_visitor.cc', rogue move?
Thanks for the feedback. PTAL https://codereview.chromium.org/1326503003/diff/700002/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/700002/net/http/bidirectional... net/http/bidirectional_stream.h:58: // No other delegate functions will be called after this. On 2015/12/02 16:36:01, mef wrote: > I don't think this is correct statement. I think we've said that OnTrailers > will be called when trailers frame arrives, not after completion of all reads > and writes. Good catch. Will do. https://codereview.chromium.org/1326503003/diff/700002/net/http/bidirectional... net/http/bidirectional_stream.h:63: virtual void OnFailed(int error) = 0; On 2015/12/02 16:36:01, mef wrote: > I think we need either OnClose, or OnSucceeded to indicate successful close of > the stream. I'd like to understand why OnClose/OnSucceeded is absolutely necessary. Even if the server half closes earlier than the client, the client will still be able to know either through onFailed (if an error occurred), or if ReadData returns 0 synchronously (everything goes well). https://codereview.chromium.org/1326503003/diff/700002/net/net.gypi File net/net.gypi (right): https://codereview.chromium.org/1326503003/diff/700002/net/net.gypi#newcode1741 net/net.gypi:1741: 'tools/quic/test_tools/mock_quic_server_session_visitor.cc', On 2015/12/02 16:36:01, mef wrote: > rogue move? I didn't move these. I rebased. Someone else probably did that.
https://codereview.chromium.org/1326503003/diff/730001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/730001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:162: if (!data_queue_.IsEmpty()) { BUG: If a former ReadData returned ERR_IO_PENDING, and then the server sends END_STREAM without data, we won't ever complete the Read (since data_queue_.IsEmpty() is true). This conditional should be removed. I will work on adding a test for this now.
Added a test which fails without the fix. I believe I have addressed all comments. PTAL. https://codereview.chromium.org/1326503003/diff/730001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/730001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:162: if (!data_queue_.IsEmpty()) { On 2015/12/07 17:35:54, xunjieli wrote: > BUG: If a former ReadData returned ERR_IO_PENDING, and then the server sends > END_STREAM without data, we won't ever complete the Read (since > data_queue_.IsEmpty() is true). This conditional should be removed. > > I will work on adding a test for this now. Done.
https://codereview.chromium.org/1326503003/diff/750001/net/http/bidirectional... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/750001/net/http/bidirectional... net/http/bidirectional_stream.cc:179: delegate_->OnFailed(ERR_SSL_CLIENT_AUTH_CERT_NEEDED); per crbug.com/558420 we should allow connection to proceed without client certificate, (in UrlRequest case it is to call request->ContinueWithCertificate(NULL); https://codereview.chromium.org/1326503003/diff/750001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/750001/net/http/bidirectional... net/http/bidirectional_stream.h:106: // TODO(xunjieli): implement a method to do flow control and a method to ping Can we also add methods that expose some metrics from SpdyStream: https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_stre... GetProtocol() raw_received_bytes() raw_sent_bytes(). https://codereview.chromium.org/1326503003/diff/750001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/750001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:81: return closed_stream_status_; Shouldn't it return 0? The comment says that it returns number of bytes read. This would explain why I was getting -137 when server was sending 2 header frames in the row. https://codereview.chromium.org/1326503003/diff/750001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:172: bool end_stream = (request_info_.method == "GET"); I suggest that we add at least 'HEAD', but I would also consider adding an explicit flag whether or not there is data to write as we have to have the same logic on Cronet Java side to figure out whether we should wait for writes.
Thanks. PTAL. https://codereview.chromium.org/1326503003/diff/750001/net/http/bidirectional... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/750001/net/http/bidirectional... net/http/bidirectional_stream.cc:179: delegate_->OnFailed(ERR_SSL_CLIENT_AUTH_CERT_NEEDED); On 2015/12/07 21:18:07, mef wrote: > per crbug.com/558420 we should allow connection to proceed without client > certificate, (in UrlRequest case it is to call > request->ContinueWithCertificate(NULL); Let's leave this for another time. If we want to do that, we need to kick off a new stream request to request a new stream and go through that flow all over again like how HttpNetworkTransaction::RestartWithCertificate does it. It is probably best to handle it in a different CL if the need arises, since this isn't as straightforward as the URLRequest's case. https://codereview.chromium.org/1326503003/diff/750001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/750001/net/http/bidirectional... net/http/bidirectional_stream.h:106: // TODO(xunjieli): implement a method to do flow control and a method to ping On 2015/12/07 21:18:07, mef wrote: > Can we also add methods that expose some metrics from SpdyStream: > > https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_stre... > > GetProtocol() > raw_received_bytes() > raw_sent_bytes(). Done. https://codereview.chromium.org/1326503003/diff/750001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/750001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:81: return closed_stream_status_; On 2015/12/07 21:18:07, mef wrote: > Shouldn't it return 0? > The comment says that it returns number of bytes read. > This would explain why I was getting -137 when server was sending 2 header > frames in the row. It should return a non-negative number that indicates the number of bytes read or a negative number (e.g. ERR_IO_PENDING) if any error occurred. I have changed the comment accordingly. https://codereview.chromium.org/1326503003/diff/750001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:172: bool end_stream = (request_info_.method == "GET"); On 2015/12/07 21:18:07, mef wrote: > I suggest that we add at least 'HEAD', but I would also consider adding an > explicit flag whether or not there is data to write as we have to have the same > logic on Cronet Java side to figure out whether we should wait for writes. Done.
Some comments. I have some time again to help out on this. Or at least spam you with comments, depending on your perspective. :) https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.cc:36: session->GetNpnProtos(&server_ssl_config.npn_protos); Hrm...This seems weird - the ssl_config_service only has part of the actual ssl_config... Well beyond the scop of this CL, I suppose. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.cc:40: FROM_HERE, base::Bind(&Delegate::OnFailed, base::Unretained(delegate_), include base/bind, base/location. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.cc:52: this, net_log_)); Should double-check this can't fail and invoke the callback synchronously. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.cc:99: DCHECK(delegate_); Get rid of this, and move it into the constructor? (Could keep it here, too, if you want, but not sure it gets us much in terms of error checking or documentation, since it can't be changed after creation) https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:8: #include "base/basictypes.h" Not needed. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:28: // ReadData or SendData should be in flight until the operation completes "one ReadData or SendData" is a bit ambiguous. Could mean one of each, or one total. Maybe "Note that at most one each of ReadData or SendData should...". Alternatively, just move the comment above ReadData / SendData, and make each comment exclusive to what it appears above. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:29: // synchronously or asynchronously. The BidirectionalStream must be torn down Suggest removing the "synchronously or asynchronously" comment - don't think it adds anything here. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:42: virtual void OnRequestHeadersSent() = 0; Should either remove request from this, or add "Response" to the two headers received methods. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:47: virtual void OnHeaders(const SpdyHeaderBlock& response_headers) = 0; Should add a Received to the end, to match the Sent method. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:51: // The delegate should call BidirectionalStream::ReadData to continue should -> may? https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:56: virtual void OnDataSent() = 0; OnSendCompleted, to match with OnReadCompleted? Or could rename OnReadCompleted to OnDataRead(). Should just be consistent. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:59: virtual void OnTrailers(const SpdyHeaderBlock& trailers) = 0; Most other methods here end with a verb (OnHeaders is the other exception). Add a Received, to make it consistent. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:61: // Called when the stream is closed or an error occurred. Is it called when Cancel() is called? https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:91: // Destroys the helper and cancels any pending request. --helper https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:103: void SendData(IOBuffer* data, int length, bool end_stream); Can either of these be called before OnHeaders? What about before OnRequestHeadersSent? https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:107: void Cancel(); I guess there's no such thing as successful completion, since we don't send/receive content-length headers. Maybe we should call this Close() instead? Also document about any in progress operations (Reads, writes, etc). Can't make promises about pending writes, but can make promises about invoking callbacks. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:108: Should we have a SetPriority method? Suppose if the underlying API we're implementing doesn't have it, we may not need one. If we don't need one, can make priority_ const. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:111: NextProto GetProtocol() const; Should mention that none of these getters may be called until after headers have been received. Or just make them all return something reasonable in that case. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:116: int64_t GetTotalReceivedBytes() const; include stdint. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:124: // TODO(xunjieli): implement a method to do flow control and a method to ping nit: implement -> Implement https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:166: BoundNetLog net_log_; const https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:168: Delegate* delegate_; Delegate* const delegate_; https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:54: void OnReadCompleted(int bytes_read) override { Check we haven't received trailers yet? https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:61: void OnDataSent() override {} No tests at this layer send any data. Add some? https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:63: void OnTrailers(const SpdyHeaderBlock& trailers) override { Should we check for re-entrancy in the callbacks? https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:80: loop_.Run(); "CreateBidirectionalStream" does not indicate "Enters a message loop and spins it until the response is complete". Maybe call it RunTest or somesuch? Or have a separate method to start spinning the message loop. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:96: // can be read synchronously. nit: Grammar. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:104: if (rv == OK) No tests simulate async reads / writes, or multiple writes...Should they? https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:110: int bytes_read_; This is just data_received_.size(), right? Can we just get rid of it? https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:208: CreateMockRead(*trailers, 4), MockRead(SYNCHRONOUS, 0, 5) // EOF Should we check async reads? Requests without trailers? (Including sync and async connection closes).
Patchset #22 (id:790001) has been deleted
Thanks for the feedback! More comments the merrier :) Thanks for taking time to review this. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.cc:36: session->GetNpnProtos(&server_ssl_config.npn_protos); On 2015/12/08 22:58:34, mmenke wrote: > Hrm...This seems weird - the ssl_config_service only has part of the actual > ssl_config... Well beyond the scop of this CL, I suppose. Yea. I had to find out about this in the hard way. Took the whole afternoon doing Wireshark to find out that the ALPN advertised is wrong. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.cc:40: FROM_HERE, base::Bind(&Delegate::OnFailed, base::Unretained(delegate_), On 2015/12/08 22:58:34, mmenke wrote: > include base/bind, base/location. Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.cc:52: this, net_log_)); On 2015/12/08 22:58:34, mmenke wrote: > Should double-check this can't fail and invoke the callback synchronously. How do we invoke the callback synchronously? Should we block until we get the callback that the stream job is created here in the constructor? https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.cc:99: DCHECK(delegate_); On 2015/12/08 22:58:34, mmenke wrote: > Get rid of this, and move it into the constructor? > > (Could keep it here, too, if you want, but not sure it gets us much in terms of > error checking or documentation, since it can't be changed after creation) Done. Good idea! https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:8: #include "base/basictypes.h" On 2015/12/08 22:58:34, mmenke wrote: > Not needed. Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:28: // ReadData or SendData should be in flight until the operation completes On 2015/12/08 22:58:34, mmenke wrote: > "one ReadData or SendData" is a bit ambiguous. Could mean one of each, or one > total. Maybe "Note that at most one each of ReadData or SendData should...". > Alternatively, just move the comment above ReadData / SendData, and make each > comment exclusive to what it appears above. Done. I edited it here. The ReadData/SendData methods also have comments which say about the same thing. Thanks! https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:29: // synchronously or asynchronously. The BidirectionalStream must be torn down On 2015/12/08 22:58:34, mmenke wrote: > Suggest removing the "synchronously or asynchronously" comment - don't think it > adds anything here. Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:42: virtual void OnRequestHeadersSent() = 0; On 2015/12/08 22:58:34, mmenke wrote: > Should either remove request from this, or add "Response" to the two headers > received methods. Done. Good idea! https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:47: virtual void OnHeaders(const SpdyHeaderBlock& response_headers) = 0; On 2015/12/08 22:58:34, mmenke wrote: > Should add a Received to the end, to match the Sent method. Done. Good idea! https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:51: // The delegate should call BidirectionalStream::ReadData to continue On 2015/12/08 22:58:34, mmenke wrote: > should -> may? Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:56: virtual void OnDataSent() = 0; On 2015/12/08 22:58:34, mmenke wrote: > OnSendCompleted, to match with OnReadCompleted? Or could rename OnReadCompleted > to OnDataRead(). Should just be consistent. Done. Great suggestion. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:59: virtual void OnTrailers(const SpdyHeaderBlock& trailers) = 0; On 2015/12/08 22:58:34, mmenke wrote: > Most other methods here end with a verb (OnHeaders is the other exception). Add > a Received, to make it consistent. Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:61: // Called when the stream is closed or an error occurred. On 2015/12/08 22:58:34, mmenke wrote: > Is it called when Cancel() is called? No, it's not called when Cancel() is called. I edited comment on Cancel() method. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:91: // Destroys the helper and cancels any pending request. On 2015/12/08 22:58:34, mmenke wrote: > --helper Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:103: void SendData(IOBuffer* data, int length, bool end_stream); On 2015/12/08 22:58:34, mmenke wrote: > Can either of these be called before OnHeaders? What about before > OnRequestHeadersSent? No. SpdyStream::SendData will check that the stream is opened (which is only set right before calling delegate->OnRequestHeadersSent). ReadData also has a CHECK to make sure the stream isn't idle. Good point. I have added comment. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:107: void Cancel(); On 2015/12/08 22:58:34, mmenke wrote: > I guess there's no such thing as successful completion, since we don't > send/receive content-length headers. Maybe we should call this Close() instead? There's actually a notion of successful completion if both sides half closes without an error. The server half closes by sending us END_STREAM, client can half closes by sending the server END_STREAM. We probably shouldn't call this Close() to avoid confuse this with half close. > Also document about any in progress operations (Reads, writes, etc). Can't make > promises about pending writes, but can make promises about invoking callbacks. Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:108: On 2015/12/08 22:58:34, mmenke wrote: > Should we have a SetPriority method? Suppose if the underlying API we're > implementing doesn't have it, we may not need one. If we don't need one, can > make priority_ const. Done. I think we only need to pass it during request time. Don't think we need to set priority afterwards right now. I have added const. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:111: NextProto GetProtocol() const; On 2015/12/08 22:58:34, mmenke wrote: > Should mention that none of these getters may be called until after headers have > been received. Or just make them all return something reasonable in that case. Done. Added comment. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:116: int64_t GetTotalReceivedBytes() const; On 2015/12/08 22:58:34, mmenke wrote: > include stdint. Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:124: // TODO(xunjieli): implement a method to do flow control and a method to ping On 2015/12/08 22:58:34, mmenke wrote: > nit: implement -> Implement Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:166: BoundNetLog net_log_; On 2015/12/08 22:58:34, mmenke wrote: > const Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:168: Delegate* delegate_; On 2015/12/08 22:58:34, mmenke wrote: > Delegate* const delegate_; Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:54: void OnReadCompleted(int bytes_read) override { On 2015/12/08 22:58:34, mmenke wrote: > Check we haven't received trailers yet? Actually we aren't delaying OnTrailers event until read completes, so it could be called before OnReadCompleted is called (for instance, if the embedder hasn't called ReadData) https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:61: void OnDataSent() override {} On 2015/12/08 22:58:34, mmenke wrote: > No tests at this layer send any data. Add some? There's one test, SendPostRequest, that sends data. I have more in bidirectional_stream_spdy_job_unittest.cc https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:63: void OnTrailers(const SpdyHeaderBlock& trailers) override { On 2015/12/08 22:58:35, mmenke wrote: > Should we check for re-entrancy in the callbacks? That sounds like something we should do. How should I check re-entrancy? https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:80: loop_.Run(); On 2015/12/08 22:58:35, mmenke wrote: > "CreateBidirectionalStream" does not indicate "Enters a message loop and spins > it until the response is complete". Maybe call it RunTest or somesuch? Or have > a separate method to start spinning the message loop. Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:96: // can be read synchronously. On 2015/12/08 22:58:34, mmenke wrote: > nit: Grammar. Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:104: if (rv == OK) On 2015/12/08 22:58:35, mmenke wrote: > No tests simulate async reads / writes, or multiple writes...Should they? Those are in bidirectional_stream_spdy_job_unittest.cc, I don't know if I should duplicate the test cases, I just want to make sure the wrapper works. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:110: int bytes_read_; On 2015/12/08 22:58:35, mmenke wrote: > This is just data_received_.size(), right? Can we just get rid of it? Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:208: CreateMockRead(*trailers, 4), MockRead(SYNCHRONOUS, 0, 5) // EOF On 2015/12/08 22:58:34, mmenke wrote: > Should we check async reads? Requests without trailers? (Including sync and > async connection closes). Those are in bidirectional_stream_spdy_job_unittest.cc. Not sure if I should duplicate the test cases.
Quick responses. Plan to do another pass later today. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.cc:52: this, net_log_)); On 2015/12/10 23:25:50, xunjieli wrote: > On 2015/12/08 22:58:34, mmenke wrote: > > Should double-check this can't fail and invoke the callback synchronously. > > How do we invoke the callback synchronously? Should we block until we get the > callback that the stream job is created here in the constructor? Sorry, I mean should double check that no HttpStreamFcatoryImpl paths invoke their callbacks synchronously. I'm pretty sure that's the case, but best to double check it. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:107: void Cancel(); On 2015/12/10 23:25:50, xunjieli wrote: > On 2015/12/08 22:58:34, mmenke wrote: > > I guess there's no such thing as successful completion, since we don't > > send/receive content-length headers. Maybe we should call this Close() > instead? > > There's actually a notion of successful completion if both sides half closes > without an error. The server half closes by sending us END_STREAM, client can > half closes by sending the server END_STREAM. > We probably shouldn't call this Close() to avoid confuse this with half close. > > Also document about any in progress operations (Reads, writes, etc). Can't > make > > promises about pending writes, but can make promises about invoking callbacks. > Done. There's no way to tell this class to send the server END_STREAM. Can only the server initiate graceful shutdown? https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:54: void OnReadCompleted(int bytes_read) override { On 2015/12/10 23:25:50, xunjieli wrote: > On 2015/12/08 22:58:34, mmenke wrote: > > Check we haven't received trailers yet? > > Actually we aren't delaying OnTrailers event until read completes, so it could > be called before OnReadCompleted is called (for instance, if the embedder hasn't > called ReadData) That seems a bit weird. Document it? Or change the interface. What does the GRPC interface do? https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:63: void OnTrailers(const SpdyHeaderBlock& trailers) override { On 2015/12/10 23:25:51, xunjieli wrote: > On 2015/12/08 22:58:35, mmenke wrote: > > Should we check for re-entrancy in the callbacks? > > That sounds like something we should do. How should I check re-entrancy? I may have had something more clever in mind yesterday (I'm sure I did! Something awe-inspiringly brilliant, honest!), but you can just set a class variable before calling into stream_, and clear it on return, and DCHECK on that. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:208: CreateMockRead(*trailers, 4), MockRead(SYNCHRONOUS, 0, 5) // EOF On 2015/12/10 23:25:51, xunjieli wrote: > On 2015/12/08 22:58:34, mmenke wrote: > > Should we check async reads? Requests without trailers? (Including sync and > > async connection closes). > > Those are in bidirectional_stream_spdy_job_unittest.cc. Not sure if I should > duplicate the test cases. Others may disagree, but I actually think if we're going to only do one or the other (And I agree that doing both here is probably overkill), it's better to test the interface used by external consumers directly, rather than the code that backs it. https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream.cc:197: NOTREACHED(); Why can't this be reached? https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream.h:80: GURL url; include GURL https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:65: CHECK_NE(OK, error); CHECK_EQ(OK, error_);? Or can we get multiple errors (A read failure and a write failure? That seems like it would be confusing, if we allowed it). https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:86: const SpdyHeaderBlock trailers() const { return trailers_; } Return const references?
https://codereview.chromium.org/1326503003/diff/750001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/750001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:172: bool end_stream = (request_info_.method == "GET"); On 2015/12/08 16:08:46, xunjieli wrote: > On 2015/12/07 21:18:07, mef wrote: > > I suggest that we add at least 'HEAD', but I would also consider adding an > > explicit flag whether or not there is data to write as we have to have the > same > > logic on Cronet Java side to figure out whether we should wait for writes. > > Done. Per our chat I've meant that we need to pass an explicit 'end_stream' flag.
https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream.cc:90: DCHECK(stream_job_); Would it make sense to return 0 if there is no stream_job_? We need to report number of bytes sent / received for failed requests, but in some failure scenarios (like no response, or http/1.1 server) there will be no stream_job_, and there isn't an easy way to check that from upper layer.
Want to see if we can simplify the job's state machine a bit. Seems more complicate than necessary. https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... File net/http/bidirectional_stream_job.h (right): https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream_job.h:8: #include "base/basictypes.h" nit: Not needed. https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream_job.h:106: virtual int64_t GetTotalSentBytes() const = 0; include <stdint.h> https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:8: #include "base/memory/scoped_ptr.h" Nit: This should be in the header. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:19: const base::TimeDelta kBufferTime = base::TimeDelta::FromMilliseconds(1); global must be POD types. Standard way to do this is kBufferTimeMs = 1, and to use TimeDelta::FromMilliseconds(kBufferTimeMs) where you're currently using kBufferTime https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:25: timer_(timer.release()), std::move(timer) https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:34: BidirectionalStreamSpdyJob::BidirectionalStreamSpdyJob( definition order should match delcaration order. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:41: if (stream_.get()) { nit: .get() not needed. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:43: DCHECK(!stream_.get()); nit: .get() not needed. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:43: DCHECK(!stream_.get()); Am I missing something, or can this not currently be safely done while in OnDataSent() / OnRequestHeadersSent()? Seems worth documenting in the parent class, and the exported class as well. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:78: CHECK(stream_); All these CHECKs should be DCHECKs. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:154: if (buffer) { Maybe early return if NULL? https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:155: data_queue_.Enqueue(buffer.Pass()); std::move https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:155: data_queue_.Enqueue(buffer.Pass()); What about flow control? I'm not familiar with the SPDY code, but what's to either pevent this buffer from getting so large we OOM or die? Or if flow control is enabled, do we need to tell the stream when we actually consume data from the queue? https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:159: ScheduleBufferedRead(); Can we just not make this call if data_queue_ was non-empty when this call was made, and then remove ScheduleBufferedRead's logic to check if the timer is already scheduled? https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:206: stream_->SendRequestHeaders(headers.Pass(), MORE_DATA_TO_SEND); std::move https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:210: DCHECK_NE(ERR_IO_PENDING, rv); nit: include base/logging.h https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:229: timer_->Start(FROM_HERE, kBufferTime, nit: Include base/location.h for FROM_HERE. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:236: // If the stream errored out, do not complete the read. Should we just DCHECK on this, and not allow it, instead? We generally don't let clients do things they should know ill fail, in net/. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:244: if (more_read_data_pending_ && ShouldWaitForMoreBufferedData()) { Why do we need more_read_data_pending_? Can't we just check the state of data_queue_? https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.h (right): https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:30: BidirectionalStreamSpdyJob(const base::WeakPtr<SpdySession>& spdy_session); explicit https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:34: scoped_ptr<base::Timer> timer); Mention what the timer does? https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:35: ~BidirectionalStreamSpdyJob() override; Seems weird to have a line break between the two constructions, but not the second constructor and the destructor. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:69: void OnClose(int status) override; Generally no blank lines are put between different methods that implement the same parent class. Not universally done, but often enough that I, at least, have grown used to it. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:89: int user_buffer_len_; read_buffer_ / read_buffer_len_? https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:100: int64_t closed_stream_sent_bytes_; include <stdint.h>
Patchset #23 (id:830001) has been deleted
Please excuse me if I missed any of your comments or misunderstood your suggestions. Happy weekend! PTAL next week when you get a chance. https://codereview.chromium.org/1326503003/diff/750001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/750001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:172: bool end_stream = (request_info_.method == "GET"); On 2015/12/11 20:06:12, mef wrote: > On 2015/12/08 16:08:46, xunjieli wrote: > > On 2015/12/07 21:18:07, mef wrote: > > > I suggest that we add at least 'HEAD', but I would also consider adding an > > > explicit flag whether or not there is data to write as we have to have the > > same > > > logic on Cronet Java side to figure out whether we should wait for writes. > > > > Done. > > Per our chat I've meant that we need to pass an explicit 'end_stream' flag. Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.cc:52: this, net_log_)); On 2015/12/11 16:42:53, mmenke wrote: > On 2015/12/10 23:25:50, xunjieli wrote: > > On 2015/12/08 22:58:34, mmenke wrote: > > > Should double-check this can't fail and invoke the callback synchronously. > > > > How do we invoke the callback synchronously? Should we block until we get the > > callback that the stream job is created here in the constructor? > > Sorry, I mean should double check that no HttpStreamFcatoryImpl paths invoke > their callbacks synchronously. I'm pretty sure that's the case, but best to > double check it. Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream.h:107: void Cancel(); On 2015/12/11 16:42:53, mmenke wrote: > On 2015/12/10 23:25:50, xunjieli wrote: > > On 2015/12/08 22:58:34, mmenke wrote: > > > I guess there's no such thing as successful completion, since we don't > > > send/receive content-length headers. Maybe we should call this Close() > > instead? > > > > There's actually a notion of successful completion if both sides half closes > > without an error. The server half closes by sending us END_STREAM, client can > > half closes by sending the server END_STREAM. > > We probably shouldn't call this Close() to avoid confuse this with half close. > > > Also document about any in progress operations (Reads, writes, etc). Can't > > make > > > promises about pending writes, but can make promises about invoking > callbacks. > > Done. > > There's no way to tell this class to send the server END_STREAM. Can only the > server initiate graceful shutdown? No. The client initiate the half close by invoking SendData with end_stream = true. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:54: void OnReadCompleted(int bytes_read) override { On 2015/12/11 16:42:53, mmenke wrote: > On 2015/12/10 23:25:50, xunjieli wrote: > > On 2015/12/08 22:58:34, mmenke wrote: > > > Check we haven't received trailers yet? > > > > Actually we aren't delaying OnTrailers event until read completes, so it could > > be called before OnReadCompleted is called (for instance, if the embedder > hasn't > > called ReadData) > > That seems a bit weird. Document it? Or change the interface. What does the > GRPC interface do? gRPC does not require the underlying implemenation to delay OnTrailers after read completes. I have added a comment. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:63: void OnTrailers(const SpdyHeaderBlock& trailers) override { On 2015/12/11 16:42:53, mmenke wrote: > On 2015/12/10 23:25:51, xunjieli wrote: > > On 2015/12/08 22:58:35, mmenke wrote: > > > Should we check for re-entrancy in the callbacks? > > > > That sounds like something we should do. How should I check re-entrancy? > > I may have had something more clever in mind yesterday (I'm sure I did! > Something awe-inspiringly brilliant, honest!), but you can just set a class > variable before calling into stream_, and clear it on return, and DCHECK on > that. Done. https://codereview.chromium.org/1326503003/diff/770001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:208: CreateMockRead(*trailers, 4), MockRead(SYNCHRONOUS, 0, 5) // EOF On 2015/12/11 16:42:53, mmenke wrote: > On 2015/12/10 23:25:51, xunjieli wrote: > > On 2015/12/08 22:58:34, mmenke wrote: > > > Should we check async reads? Requests without trailers? (Including sync > and > > > async connection closes). > > > > Those are in bidirectional_stream_spdy_job_unittest.cc. Not sure if I should > > duplicate the test cases. > > Others may disagree, but I actually think if we're going to only do one or the > other (And I agree that doing both here is probably overkill), it's better to > test the interface used by external consumers directly, rather than the code > that backs it. Done. I think your idea is better. If we add QUIC support, we can make the test run a second time with QUIC, and we won't duplicate test code. https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream.cc:90: DCHECK(stream_job_); On 2015/12/11 20:54:26, mef wrote: > Would it make sense to return 0 if there is no stream_job_? We need to report > number of bytes sent / received for failed requests, but in some failure > scenarios (like no response, or http/1.1 server) there will be no stream_job_, > and there isn't an easy way to check that from upper layer. Done. https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream.cc:197: NOTREACHED(); On 2015/12/11 16:42:53, mmenke wrote: > Why can't this be reached? Not sure how to handle this. HttpNetworkTransaction still lets client to read the body of the response. I think we could surface the error code directly. Done. https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream.h:80: GURL url; On 2015/12/11 16:42:53, mmenke wrote: > include GURL Done. https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... File net/http/bidirectional_stream_job.h (right): https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream_job.h:8: #include "base/basictypes.h" On 2015/12/11 22:19:58, mmenke wrote: > nit: Not needed. Done. https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream_job.h:106: virtual int64_t GetTotalSentBytes() const = 0; On 2015/12/11 22:19:58, mmenke wrote: > include <stdint.h> Done. https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:65: CHECK_NE(OK, error); On 2015/12/11 16:42:53, mmenke wrote: > CHECK_EQ(OK, error_);? Or can we get multiple errors (A read failure and a > write failure? That seems like it would be confusing, if we allowed it). Done. I don't think we can get multiple OnFailed callback. The SpdyStream::OnClose is guaranteed to be called once, we basically translate that into OnFailed, so I don't think it can get invoked multiple times. https://codereview.chromium.org/1326503003/diff/810001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:86: const SpdyHeaderBlock trailers() const { return trailers_; } On 2015/12/11 16:42:53, mmenke wrote: > Return const references? Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:8: #include "base/memory/scoped_ptr.h" On 2015/12/11 22:19:58, mmenke wrote: > Nit: This should be in the header. Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:19: const base::TimeDelta kBufferTime = base::TimeDelta::FromMilliseconds(1); On 2015/12/11 22:19:59, mmenke wrote: > global must be POD types. Standard way to do this is kBufferTimeMs = 1, and to > use TimeDelta::FromMilliseconds(kBufferTimeMs) where you're currently using > kBufferTime Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:25: timer_(timer.release()), On 2015/12/11 22:19:58, mmenke wrote: > std::move(timer) Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:34: BidirectionalStreamSpdyJob::BidirectionalStreamSpdyJob( On 2015/12/11 22:19:59, mmenke wrote: > definition order should match delcaration order. Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:41: if (stream_.get()) { On 2015/12/11 22:19:59, mmenke wrote: > nit: .get() not needed. Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:43: DCHECK(!stream_.get()); On 2015/12/11 22:19:58, mmenke wrote: > nit: .get() not needed. Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:43: DCHECK(!stream_.get()); On 2015/12/11 22:19:58, mmenke wrote: > Am I missing something, or can this not currently be safely done while in > OnDataSent() / OnRequestHeadersSent()? Seems worth documenting in the parent > class, and the exported class as well. Why is it unsafe? The delegate can't delete the job in OnDataSent callback? https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:78: CHECK(stream_); On 2015/12/11 22:19:58, mmenke wrote: > All these CHECKs should be DCHECKs. Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:154: if (buffer) { On 2015/12/11 22:19:58, mmenke wrote: > Maybe early return if NULL? Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:155: data_queue_.Enqueue(buffer.Pass()); On 2015/12/11 22:19:59, mmenke wrote: > std::move Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:155: data_queue_.Enqueue(buffer.Pass()); On 2015/12/11 22:19:59, mmenke wrote: > What about flow control? > > I'm not familiar with the SPDY code, but what's to either pevent this buffer > from getting so large we OOM or die? Or if flow control is enabled, do we need > to tell the stream when we actually consume data from the queue? Good question. I added comment. I believe that SpdyBuffer that we received has a ConsumeCallback hooked up that it would adjust recv window flow: buffer->AddConsumeCallback( base::Bind(&SpdyStream::OnReadBufferConsumed, GetWeakPtr())); https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:159: ScheduleBufferedRead(); On 2015/12/11 22:19:58, mmenke wrote: > Can we just not make this call if data_queue_ was non-empty when this call was > made, and then remove ScheduleBufferedRead's logic to check if the timer is > already scheduled? There is one case where we still need to invoke OnDataReceived even though the data_queue is empty. If client calls ReadData, but there is no data buffered, the call will return IO_PENDING. If the server sends us END_STREAM with empty buffer after this, we won't enqueue anything to the data_queue_, but we still should invoke the completion callback here. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:206: stream_->SendRequestHeaders(headers.Pass(), MORE_DATA_TO_SEND); On 2015/12/11 22:19:58, mmenke wrote: > std::move Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:210: DCHECK_NE(ERR_IO_PENDING, rv); On 2015/12/11 22:19:58, mmenke wrote: > nit: include base/logging.h Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:229: timer_->Start(FROM_HERE, kBufferTime, On 2015/12/11 22:19:58, mmenke wrote: > nit: Include base/location.h for FROM_HERE. Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:236: // If the stream errored out, do not complete the read. On 2015/12/11 22:19:59, mmenke wrote: > Should we just DCHECK on this, and not allow it, instead? We generally don't > let clients do things they should know ill fail, in net/. Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:244: if (more_read_data_pending_ && ShouldWaitForMoreBufferedData()) { On 2015/12/11 22:19:58, mmenke wrote: > Why do we need more_read_data_pending_? Can't we just check the state of > data_queue_? I was copying SpdyHttpStream's logic. The ShouldWaitForMoreBufferedData does check the state of data_queue, not sure what else we can check so we can substitue this more_read_data_pending_. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.h (right): https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:30: BidirectionalStreamSpdyJob(const base::WeakPtr<SpdySession>& spdy_session); On 2015/12/11 22:19:59, mmenke wrote: > explicit Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:34: scoped_ptr<base::Timer> timer); On 2015/12/11 22:19:59, mmenke wrote: > Mention what the timer does? Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:35: ~BidirectionalStreamSpdyJob() override; On 2015/12/11 22:19:59, mmenke wrote: > Seems weird to have a line break between the two constructions, but not the > second constructor and the destructor. Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:69: void OnClose(int status) override; On 2015/12/11 22:19:59, mmenke wrote: > Generally no blank lines are put between different methods that implement the > same parent class. Not universally done, but often enough that I, at least, > have grown used to it. Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:89: int user_buffer_len_; On 2015/12/11 22:19:59, mmenke wrote: > read_buffer_ / read_buffer_len_? Done. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:100: int64_t closed_stream_sent_bytes_; On 2015/12/11 22:19:59, mmenke wrote: > include <stdint.h> Done.
Patchset #23 (id:850001) has been deleted
Looks pretty good. I've synced https://codereview.chromium.org/1412243012 on top of this and it works for GET, HEAD and POST. I would suggest adding more tests for sending data. https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... net/http/bidirectional_stream.h:38: // TODO(xunjieli): Surface the protocol negotiated. This TODO is done. https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... net/http/bidirectional_stream.h:43: // Called when headers have been sent. Add 'This is called at most once for the lifetime of a stream.'? https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... net/http/bidirectional_stream.h:112: // Getters that should only be called after headers have been received: I would expected them to work after headers are sent but before they have been received. https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... File net/http/bidirectional_stream_request_info.h (right): https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... net/http/bidirectional_stream_request_info.h:14: namespace net { nit: add newline. https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... net/http/bidirectional_stream_request_info.h:15: struct NET_EXPORT BidirectionalStreamRequestInfo { I think we need a description for publicly exported class. https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:74: on_read_complete_count_++; nit: C++ coding standard suggests using prefix increments like '++on_read_complete_count_'. Here and elsewhere. https://codereview.chromium.org/1326503003/diff/870001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/870001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:246: // Check to see that the stream has not errored outd. nit: outd.
Thanks for the comments. I will think about what test cases I can add for sending data. Appreciate any suggestion or more comments in the meanwhile. Thanks! https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... net/http/bidirectional_stream.h:38: // TODO(xunjieli): Surface the protocol negotiated. On 2015/12/14 17:59:00, mef wrote: > This TODO is done. Done. https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... net/http/bidirectional_stream.h:43: // Called when headers have been sent. On 2015/12/14 17:59:01, mef wrote: > Add 'This is called at most once for the lifetime of a stream.'? Done. https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... net/http/bidirectional_stream.h:112: // Getters that should only be called after headers have been received: On 2015/12/14 17:59:00, mef wrote: > I would expected them to work after headers are sent but before they have been > received. Done. I changed the code accordingly. https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... File net/http/bidirectional_stream_request_info.h (right): https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... net/http/bidirectional_stream_request_info.h:14: namespace net { On 2015/12/14 17:59:01, mef wrote: > nit: add newline. Done. https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... net/http/bidirectional_stream_request_info.h:15: struct NET_EXPORT BidirectionalStreamRequestInfo { On 2015/12/14 17:59:01, mef wrote: > I think we need a description for publicly exported class. Done. https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/870001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:74: on_read_complete_count_++; On 2015/12/14 17:59:01, mef wrote: > nit: C++ coding standard suggests using prefix increments like > '++on_read_complete_count_'. Here and elsewhere. Done. Although according to Google C++ style guide, we can use either with non-objects. https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement Preincement in preferred in iterators and other template objects. https://codereview.chromium.org/1326503003/diff/870001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/870001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:246: // Check to see that the stream has not errored outd. On 2015/12/14 17:59:01, mef wrote: > nit: outd. Done.
https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:43: DCHECK(!stream_.get()); On 2015/12/11 23:48:40, xunjieli wrote: > On 2015/12/11 22:19:58, mmenke wrote: > > Am I missing something, or can this not currently be safely done while in > > OnDataSent() / OnRequestHeadersSent()? Seems worth documenting in the parent > > class, and the exported class as well. > > Why is it unsafe? The delegate can't delete the job in OnDataSent callback? Sorry, I wasn't remotely clear there. See: https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_stre... https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:155: data_queue_.Enqueue(buffer.Pass()); On 2015/12/11 23:48:40, xunjieli wrote: > On 2015/12/11 22:19:59, mmenke wrote: > > What about flow control? > > > > I'm not familiar with the SPDY code, but what's to either pevent this buffer > > from getting so large we OOM or die? Or if flow control is enabled, do we > need > > to tell the stream when we actually consume data from the queue? > > Good question. I added comment. I believe that SpdyBuffer that we received has a > ConsumeCallback hooked up that it would adjust recv window flow: > > buffer->AddConsumeCallback( > base::Bind(&SpdyStream::OnReadBufferConsumed, GetWeakPtr())); I'll dig into this a bit before responding. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:159: ScheduleBufferedRead(); On 2015/12/11 23:48:40, xunjieli wrote: > On 2015/12/11 22:19:58, mmenke wrote: > > Can we just not make this call if data_queue_ was non-empty when this call was > > made, and then remove ScheduleBufferedRead's logic to check if the timer is > > already scheduled? > > There is one case where we still need to invoke OnDataReceived even though the > data_queue is empty. > > If client calls ReadData, but there is no data buffered, the call will return > IO_PENDING. > If the server sends us END_STREAM with empty buffer after this, we won't enqueue > anything to the data_queue_, but we still should invoke the completion callback > here. Think it would actually be simpler to just have OnClose directly call into the delegate in that case. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:244: if (more_read_data_pending_ && ShouldWaitForMoreBufferedData()) { On 2015/12/11 23:48:41, xunjieli wrote: > On 2015/12/11 22:19:58, mmenke wrote: > > Why do we need more_read_data_pending_? Can't we just check the state of > > data_queue_? > > I was copying SpdyHttpStream's logic. The ShouldWaitForMoreBufferedData does > check the state of data_queue, not sure what else we can check so we can > substitue this more_read_data_pending_. Hrm...I think the extra variable here makes this code more confusing than strictly necessary. The more variables you have whose states can be derived from other variables, the easier it is to end up in an inconsistent state where something isn't being updated correctly. https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:701: deterministic_data_->RunFor(5); Suggest using SequencedSocketData instead. We want to get rid of DeterministicSocketData, as SequencedSocketData is easier to use. While RunFor can let you probe objects at intermediate steps, which SequencedSocketData does not currently support, the fact is that no one takes advantage of that. https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:702: base::RunLoop().RunUntilIdle(); In my experience, RunUntilIdle makes for flaky, regression prone tests. Ideally you should be using RunLoop::Run() and then RunLoop::Quit when the even you care about has happened.
https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:43: DCHECK(!stream_.get()); On 2015/12/14 19:48:37, mmenke wrote: > On 2015/12/11 23:48:40, xunjieli wrote: > > On 2015/12/11 22:19:58, mmenke wrote: > > > Am I missing something, or can this not currently be safely done while in > > > OnDataSent() / OnRequestHeadersSent()? Seems worth documenting in the > parent > > > class, and the exported class as well. > > > > Why is it unsafe? The delegate can't delete the job in OnDataSent callback? > > Sorry, I wasn't remotely clear there. > > See: > https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_stre... And I think we should have tests for destruction during callbacks, if we intend to support it.
https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:43: DCHECK(!stream_.get()); On 2015/12/14 19:52:34, mmenke wrote: > On 2015/12/14 19:48:37, mmenke wrote: > > On 2015/12/11 23:48:40, xunjieli wrote: > > > On 2015/12/11 22:19:58, mmenke wrote: > > > > Am I missing something, or can this not currently be safely done while in > > > > OnDataSent() / OnRequestHeadersSent()? Seems worth documenting in the > > parent > > > > class, and the exported class as well. > > > > > > Why is it unsafe? The delegate can't delete the job in OnDataSent callback? > > > > Sorry, I wasn't remotely clear there. > > > > See: > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_stre... > > And I think we should have tests for destruction during callbacks, if we intend > to support it. Done. Thanks for pointing me to the code. I have documented on this requirement. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:155: data_queue_.Enqueue(buffer.Pass()); On 2015/12/14 19:48:37, mmenke wrote: > On 2015/12/11 23:48:40, xunjieli wrote: > > On 2015/12/11 22:19:59, mmenke wrote: > > > What about flow control? > > > > > > I'm not familiar with the SPDY code, but what's to either pevent this buffer > > > from getting so large we OOM or die? Or if flow control is enabled, do we > > need > > > to tell the stream when we actually consume data from the queue? > > > > Good question. I added comment. I believe that SpdyBuffer that we received has > a > > ConsumeCallback hooked up that it would adjust recv window flow: > > > > buffer->AddConsumeCallback( > > base::Bind(&SpdyStream::OnReadBufferConsumed, GetWeakPtr())); > > I'll dig into this a bit before responding. Acknowledged. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:159: ScheduleBufferedRead(); On 2015/12/14 19:48:37, mmenke wrote: > On 2015/12/11 23:48:40, xunjieli wrote: > > On 2015/12/11 22:19:58, mmenke wrote: > > > Can we just not make this call if data_queue_ was non-empty when this call > was > > > made, and then remove ScheduleBufferedRead's logic to check if the timer is > > > already scheduled? > > > > There is one case where we still need to invoke OnDataReceived even though the > > data_queue is empty. > > > > If client calls ReadData, but there is no data buffered, the call will return > > IO_PENDING. > > If the server sends us END_STREAM with empty buffer after this, we won't > enqueue > > anything to the data_queue_, but we still should invoke the completion > callback > > here. > > Think it would actually be simpler to just have OnClose directly call into the > delegate in that case. We are having OnClose calling into Delegate via Delegate::OnDataRead(0) here though. We don't have a Delegate::OnClose method, which was removed for simplicity. https://codereview.chromium.org/1326503003/diff/810001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:244: if (more_read_data_pending_ && ShouldWaitForMoreBufferedData()) { On 2015/12/14 19:48:37, mmenke wrote: > On 2015/12/11 23:48:41, xunjieli wrote: > > On 2015/12/11 22:19:58, mmenke wrote: > > > Why do we need more_read_data_pending_? Can't we just check the state of > > > data_queue_? > > > > I was copying SpdyHttpStream's logic. The ShouldWaitForMoreBufferedData does > > check the state of data_queue, not sure what else we can check so we can > > substitue this more_read_data_pending_. > > Hrm...I think the extra variable here makes this code more confusing than > strictly necessary. The more variables you have whose states can be derived > from other variables, the easier it is to end up in an inconsistent state where > something isn't being updated correctly. I don't think ShouldWaitForMoreBufferedData is enough. If the ReadData is called with a large |user_buffer|, with |more_read_data_pending_| removed, we would wait until OnClose is called or |user_buffer_| is completely filled before invoking OnDataRead callback. However this is not the right behavior. Because the send data and received data can interleave. (The client can expect to receive a small amount of data, send some data, then receive some data, ... and finally to have both sides close) If we only depend on the state of the |data_queue_|, we might be postponing the read completion callback forever. https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:701: deterministic_data_->RunFor(5); On 2015/12/14 19:48:37, mmenke wrote: > Suggest using SequencedSocketData instead. We want to get rid of > DeterministicSocketData, as SequencedSocketData is easier to use. While RunFor > can let you probe objects at intermediate steps, which SequencedSocketData does > not currently support, the fact is that no one takes advantage of that. I am aware of the deprecation plan for DeterministicSocketData. But I find it very convenient to control how many frames to receive/send. Since we consolidate several DATA frames into one async callback, if using Quit/Run alone, we can't really test the buffering logic (e.g firing the timer when a particular data frame is received). https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:702: base::RunLoop().RunUntilIdle(); On 2015/12/14 19:48:37, mmenke wrote: > In my experience, RunUntilIdle makes for flaky, regression prone tests. Ideally > you should be using RunLoop::Run() and then RunLoop::Quit when the even you care > about has happened. I will try adjusting it accordingly. But I still have a question on DeterministicSocketData, see above.
[+rch]: Ryan, do you think we'll need the behavior of DeterministicData::RunFor, long term, for SPDY and QUIC tests? If so, I'd rather see the capability added to SequencedSocketData sooner rather than later, to make the conversion less involved. https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:701: deterministic_data_->RunFor(5); On 2015/12/14 21:03:05, xunjieli wrote: > On 2015/12/14 19:48:37, mmenke wrote: > > Suggest using SequencedSocketData instead. We want to get rid of > > DeterministicSocketData, as SequencedSocketData is easier to use. While > RunFor > > can let you probe objects at intermediate steps, which SequencedSocketData > does > > not currently support, the fact is that no one takes advantage of that. > > I am aware of the deprecation plan for DeterministicSocketData. But I find it > very convenient to control how many frames to receive/send. Since we consolidate > several DATA frames into one async callback, if using Quit/Run alone, we can't > really test the buffering logic (e.g firing the timer when a particular data > frame is received). Hrm... If we're going to need to be able to test this sort of thing, we may need to add this feature to sequenced socket data. It shouldn't be too hard. The reason for preferring it over DeterministicSocket data is the code is much saner, failures are much easier to debug. SequencedSocketData also forces be in sequential order, with each number only occurring once, which DeterministicSocket does not, so some very ...creative... tests can still manage to pass.
rch@chromium.org changed reviewers: + rch@chromium.org
https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... net/http/bidirectional_stream_unittest.cc:701: deterministic_data_->RunFor(5); On 2015/12/14 21:49:38, mmenke wrote: > On 2015/12/14 21:03:05, xunjieli wrote: > > On 2015/12/14 19:48:37, mmenke wrote: > > > Suggest using SequencedSocketData instead. We want to get rid of > > > DeterministicSocketData, as SequencedSocketData is easier to use. While > > RunFor > > > can let you probe objects at intermediate steps, which SequencedSocketData > > does > > > not currently support, the fact is that no one takes advantage of that. > > > > I am aware of the deprecation plan for DeterministicSocketData. But I find it > > very convenient to control how many frames to receive/send. Since we > consolidate > > several DATA frames into one async callback, if using Quit/Run alone, we can't > > really test the buffering logic (e.g firing the timer when a particular data > > frame is received). > > Hrm... If we're going to need to be able to test this sort of thing, we may > need to add this feature to sequenced socket data. It shouldn't be too hard. > The reason for preferring it over DeterministicSocket data is the code is much > saner, failures are much easier to debug. SequencedSocketData also forces be in > sequential order, with each number only occurring once, which > DeterministicSocket does not, so some very ...creative... tests can still manage > to pass. I believe that SequencedSocketData already has a mechanism to do this. It has a PAUSED state that it enters when, say, and ASYNC MockRead returns ERR_IO_PENDING. This causes the state machine to basically stop until CompleteRead() is called. I think it should be possible to use this mechanism in place of DeterministicSocketData::RunFor(). With RunFor() the stopping point is decided by the argument to RunFor(). With SequencedSocketData, the stopping point is determined by the MockRead/MockWrites. Does that sounds plausible?
On 2015/12/14 22:33:18, Ryan Hamilton wrote: > https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... > File net/http/bidirectional_stream_unittest.cc (right): > > https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... > net/http/bidirectional_stream_unittest.cc:701: deterministic_data_->RunFor(5); > On 2015/12/14 21:49:38, mmenke wrote: > > On 2015/12/14 21:03:05, xunjieli wrote: > > > On 2015/12/14 19:48:37, mmenke wrote: > > > > Suggest using SequencedSocketData instead. We want to get rid of > > > > DeterministicSocketData, as SequencedSocketData is easier to use. While > > > RunFor > > > > can let you probe objects at intermediate steps, which SequencedSocketData > > > does > > > > not currently support, the fact is that no one takes advantage of that. > > > > > > I am aware of the deprecation plan for DeterministicSocketData. But I find > it > > > very convenient to control how many frames to receive/send. Since we > > consolidate > > > several DATA frames into one async callback, if using Quit/Run alone, we > can't > > > really test the buffering logic (e.g firing the timer when a particular data > > > frame is received). > > > > Hrm... If we're going to need to be able to test this sort of thing, we may > > need to add this feature to sequenced socket data. It shouldn't be too hard. > > The reason for preferring it over DeterministicSocket data is the code is much > > saner, failures are much easier to debug. SequencedSocketData also forces be > in > > sequential order, with each number only occurring once, which > > DeterministicSocket does not, so some very ...creative... tests can still > manage > > to pass. > > I believe that SequencedSocketData already has a mechanism to do this. It has a > PAUSED state that it enters when, say, and ASYNC MockRead returns > ERR_IO_PENDING. This causes the state machine to basically stop until > CompleteRead() is called. I think it should be possible to use this mechanism in > place of DeterministicSocketData::RunFor(). > > With RunFor() the stopping point is decided by the argument to RunFor(). With > SequencedSocketData, the stopping point is determined by the > MockRead/MockWrites. > > Does that sounds plausible? I don't see how you use it. There are two possible designed here: It pauses on all async reads/writes, or you tell it which reads/writes to pause on. The first is clearly not the case, as it automatically advances async operations by default. I don't see an API for the latter. Am I missing something?
On 2015/12/14 22:44:22, mmenke wrote: > On 2015/12/14 22:33:18, Ryan Hamilton wrote: > > > https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... > > File net/http/bidirectional_stream_unittest.cc (right): > > > > > https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... > > net/http/bidirectional_stream_unittest.cc:701: deterministic_data_->RunFor(5); > > On 2015/12/14 21:49:38, mmenke wrote: > > > On 2015/12/14 21:03:05, xunjieli wrote: > > > > On 2015/12/14 19:48:37, mmenke wrote: > > > > > Suggest using SequencedSocketData instead. We want to get rid of > > > > > DeterministicSocketData, as SequencedSocketData is easier to use. While > > > > RunFor > > > > > can let you probe objects at intermediate steps, which > SequencedSocketData > > > > does > > > > > not currently support, the fact is that no one takes advantage of that. > > > > > > > > I am aware of the deprecation plan for DeterministicSocketData. But I find > > it > > > > very convenient to control how many frames to receive/send. Since we > > > consolidate > > > > several DATA frames into one async callback, if using Quit/Run alone, we > > can't > > > > really test the buffering logic (e.g firing the timer when a particular > data > > > > frame is received). > > > > > > Hrm... If we're going to need to be able to test this sort of thing, we may > > > need to add this feature to sequenced socket data. It shouldn't be too > hard. > > > The reason for preferring it over DeterministicSocket data is the code is > much > > > saner, failures are much easier to debug. SequencedSocketData also forces > be > > in > > > sequential order, with each number only occurring once, which > > > DeterministicSocket does not, so some very ...creative... tests can still > > manage > > > to pass. > > > > I believe that SequencedSocketData already has a mechanism to do this. It has > a > > PAUSED state that it enters when, say, and ASYNC MockRead returns > > ERR_IO_PENDING. This causes the state machine to basically stop until > > CompleteRead() is called. I think it should be possible to use this mechanism > in > > place of DeterministicSocketData::RunFor(). > > > > With RunFor() the stopping point is decided by the argument to RunFor(). With > > SequencedSocketData, the stopping point is determined by the > > MockRead/MockWrites. > > > > Does that sounds plausible? > > I don't see how you use it. > > There are two possible designed here: It pauses on all async reads/writes, or > you tell it which reads/writes to pause on. The first is clearly not the case, > as it automatically advances async operations by default. I don't see an API > for the latter. Am I missing something? Oh, it's an undocumented hack! You give it a read result of ERR_IO_PENDING, and then make the next operation the real one. I guess it doesn't work on writes?
On 2015/12/14 22:50:48, mmenke wrote: > On 2015/12/14 22:44:22, mmenke wrote: > > On 2015/12/14 22:33:18, Ryan Hamilton wrote: > > > > > > https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... > > > File net/http/bidirectional_stream_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/1326503003/diff/890001/net/http/bidirectional... > > > net/http/bidirectional_stream_unittest.cc:701: > deterministic_data_->RunFor(5); > > > On 2015/12/14 21:49:38, mmenke wrote: > > > > On 2015/12/14 21:03:05, xunjieli wrote: > > > > > On 2015/12/14 19:48:37, mmenke wrote: > > > > > > Suggest using SequencedSocketData instead. We want to get rid of > > > > > > DeterministicSocketData, as SequencedSocketData is easier to use. > While > > > > > RunFor > > > > > > can let you probe objects at intermediate steps, which > > SequencedSocketData > > > > > does > > > > > > not currently support, the fact is that no one takes advantage of > that. > > > > > > > > > > I am aware of the deprecation plan for DeterministicSocketData. But I > find > > > it > > > > > very convenient to control how many frames to receive/send. Since we > > > > consolidate > > > > > several DATA frames into one async callback, if using Quit/Run alone, we > > > can't > > > > > really test the buffering logic (e.g firing the timer when a particular > > data > > > > > frame is received). > > > > > > > > Hrm... If we're going to need to be able to test this sort of thing, we > may > > > > need to add this feature to sequenced socket data. It shouldn't be too > > hard. > > > > The reason for preferring it over DeterministicSocket data is the code is > > much > > > > saner, failures are much easier to debug. SequencedSocketData also forces > > be > > > in > > > > sequential order, with each number only occurring once, which > > > > DeterministicSocket does not, so some very ...creative... tests can still > > > manage > > > > to pass. > > > > > > I believe that SequencedSocketData already has a mechanism to do this. It > has > > a > > > PAUSED state that it enters when, say, and ASYNC MockRead returns > > > ERR_IO_PENDING. This causes the state machine to basically stop until > > > CompleteRead() is called. I think it should be possible to use this > mechanism > > in > > > place of DeterministicSocketData::RunFor(). > > > > > > With RunFor() the stopping point is decided by the argument to RunFor(). > With > > > SequencedSocketData, the stopping point is determined by the > > > MockRead/MockWrites. > > > > > > Does that sounds plausible? > > > > I don't see how you use it. > > > > There are two possible designed here: It pauses on all async reads/writes, or > > you tell it which reads/writes to pause on. The first is clearly not the > case, > > as it automatically advances async operations by default. I don't see an API > > for the latter. Am I missing something? > > Oh, it's an undocumented hack! You give it a read result of ERR_IO_PENDING, and > then make the next operation the real one. I guess it doesn't work on writes? Yeah, that sounds plausible. It wasn't part of the initial vision for SequenceSocketData, but either OrderedSocketData or DelayedSocketData implemented this and it seemed like it was a good idea so I subsumed it. I don't remember if it didn't work for writes, but that wouldn't surprise me. I only added whatever the other class was doing and if it only did Read, then that's probably what I did...
On 2015/12/14 23:23:38, Ryan Hamilton wrote: > Yeah, that sounds plausible. It wasn't part of the initial vision for > SequenceSocketData, but either OrderedSocketData or DelayedSocketData > implemented this and it seemed like it was a good idea so I subsumed it. I don't > remember if it didn't work for writes, but that wouldn't surprise me. I only > added whatever the other class was doing and if it only did Read, then that's > probably what I did... The current implementation is a bit messy to use - it basically requires a RunUntilIdle call to wait reach the pause (Unless it also sends a notification...Which should make the pause not needed in the first place). Going to go ahead and add a method to wait until the pause, to make things a bit neater.
On 2015/12/14 23:26:45, mmenke wrote: > On 2015/12/14 23:23:38, Ryan Hamilton wrote: > > Yeah, that sounds plausible. It wasn't part of the initial vision for > > SequenceSocketData, but either OrderedSocketData or DelayedSocketData > > implemented this and it seemed like it was a good idea so I subsumed it. I > don't > > remember if it didn't work for writes, but that wouldn't surprise me. I only > > added whatever the other class was doing and if it only did Read, then that's > > probably what I did... > > The current implementation is a bit messy to use - it basically requires a > RunUntilIdle call to wait reach the pause (Unless it also sends a > notification...Which should make the pause not needed in the first place). > Going to go ahead and add a method to wait until the pause, to make things a bit > neater. Sounds good to me!
On 2015/12/14 23:44:35, Ryan Hamilton wrote: > On 2015/12/14 23:26:45, mmenke wrote: > > On 2015/12/14 23:23:38, Ryan Hamilton wrote: > > > Yeah, that sounds plausible. It wasn't part of the initial vision for > > > SequenceSocketData, but either OrderedSocketData or DelayedSocketData > > > implemented this and it seemed like it was a good idea so I subsumed it. I > > don't > > > remember if it didn't work for writes, but that wouldn't surprise me. I only > > > added whatever the other class was doing and if it only did Read, then > that's > > > probably what I did... > > > > The current implementation is a bit messy to use - it basically requires a > > RunUntilIdle call to wait reach the pause (Unless it also sends a > > notification...Which should make the pause not needed in the first place). > > Going to go ahead and add a method to wait until the pause, to make things a > bit > > neater. > > Sounds good to me! Thanks Matt and Ryan! I got rid of DeterministicSocketData in bidirectional_stream_unittest.cc. Matt, feel free to file a bug on the refactoring and assign me if you don't get to it. PTAL.
Patchset #27 (id:950001) has been deleted
Sorry, I haven't gone through the whole thing yet, but it looks pretty good. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:31: // ReadData or SendData should be in flight until the operation completes nit: end with . https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:45: // sending data. or call BidirectionalStream::Cancel to cancel the stream. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:55: // much data is available. read -> pending read is available -> is read. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:60: // Called when the entire buffer passed through SendData is sent. Update comment similarly to OnDataRead(). https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:64: // are received, which can happen before a read completes. Maybe clarify that no action is required from the delegate? https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:72: virtual ~Delegate() {} Should {} go into .cc file? https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:78: BidirectionalStream(const BidirectionalStreamRequestInfo& request_info, need comment, especially considering that this is main constructor. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:85: BidirectionalStream(const BidirectionalStreamRequestInfo& request_info, FWIW HttpStream-related classes pass request_info as pointer to avoid copying. I think we should do the same for BidirectionalStream. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:92: // |this| should not be destroyed during Delegate::OnHeadersSent or That's an interesting comment. Why not? Do we need tests for this? https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:99: // This should not be called before Delegate::OnHeadersSent is invoked, and OnHeadersSent -> OnHeadersReceived? https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:100: // should not be called again unless it IO completes synchronously or until IO completes -> returns https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:117: // stablished, return kProtoUnknown. established (here and below). https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:173: const BidirectionalStreamRequestInfo request_info_; can this be a scoped_ptr<BidirectionalStreamRequestInfo>? If yes, and if it could take an ownership of request_info passed to constructor, then we would avoid cloning of that structure. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... File net/http/bidirectional_stream_job.h (right): https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream_job.h:82: RequestPriority priority, should priority be folded into request_info? https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream_job.h:105: // kProtoSPDYMinimumVersion and kProtoSPDYMaximumVersion. I suppose that QUIC implementation will return higher version. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... File net/http/bidirectional_stream_request_info.h (right): https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream_request_info.h:31: bool end_stream_on_headers; maybe add request_priority field here? https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:21: const int64 kBufferTimeMs = 1; would be nice to have comment. Also consider moving into namespace {} as current code means that it is public symbol net::kBufferTimeMs. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:28: more_read_data_pending_(false), initialize all fields, e.g. |read_buffer_len_| is not initialized. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:76: if (!stream_closed_) FWIW it seems that |stream_closed_| is equivalent of !stream_, am I missing something? https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.h (right): https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:41: RequestPriority priority, nit: any particular reason why priority is not a part of request_info? https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:72: bool more_read_data_pending_; move close to other read-related fields like data_queue_? https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:74: BidirectionalStreamRequestInfo request_info_; Here is another copy of BidirectionalStreamRequestInfo struct. It would be nice if original object allocated by the consumer could be passed down to here as scoped_ptr<BidirectionalStreamRequestInfo>. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:76: SpdyReadQueue data_queue_; optional nit: maybe call this read_data_queue_? https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:85: // After |stream_| has been closed, this keeps track of the total number of move these together with closed_stream_ and closed_stream_status_.
Thanks everyone for the suggestions and comments so far. I have added three more tests (one where we cancel after data is sent, one where we cancel while read is pending, and one where we surface the error from SpdyStream). Matt: Do you think this CL is close to landing? I am taking the big chunk of January off. I don't want to block Misha. I'd like to wrap things up wherever I can. Is it realistic to get this CL in shape for landing by the end of this week? Thank you.
https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:198: DCHECK_EQ(OK, status); this seems unneeded due to line 191. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:227: delegate_->OnFailed(static_cast<Error>(rv)); I think delegate_->OnFailed now takes int.
Patchset #28 (id:990001) has been deleted
Thank you for the review! PTAL. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:31: // ReadData or SendData should be in flight until the operation completes On 2015/12/15 22:59:40, mef wrote: > nit: end with . Done. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:45: // sending data. On 2015/12/15 22:59:39, mef wrote: > or call BidirectionalStream::Cancel to cancel the stream. It's illegal to call Cancel during OnHeadersSent or OnDataSent callback. (See https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_stre...) I have updated the comments. Thanks for bring this up. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:55: // much data is available. On 2015/12/15 22:59:40, mef wrote: > read -> pending read > is available -> is read. Done. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:60: // Called when the entire buffer passed through SendData is sent. On 2015/12/15 22:59:40, mef wrote: > Update comment similarly to OnDataRead(). Done. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:64: // are received, which can happen before a read completes. On 2015/12/15 22:59:40, mef wrote: > Maybe clarify that no action is required from the delegate? Done. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:72: virtual ~Delegate() {} On 2015/12/15 22:59:40, mef wrote: > Should {} go into .cc file? Done. Good catch. Thanks! https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:78: BidirectionalStream(const BidirectionalStreamRequestInfo& request_info, On 2015/12/15 22:59:40, mef wrote: > need comment, especially considering that this is main constructor. Done. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:85: BidirectionalStream(const BidirectionalStreamRequestInfo& request_info, On 2015/12/15 22:59:39, mef wrote: > FWIW HttpStream-related classes pass request_info as pointer to avoid copying. I > think we should do the same for BidirectionalStream. Done. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:92: // |this| should not be destroyed during Delegate::OnHeadersSent or On 2015/12/15 22:59:40, mef wrote: > That's an interesting comment. Why not? Do we need tests for this? It's illegal to call Cancel during OnHeadersSent or OnDataSent callback. (See https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_stre...) Not sure how this can be tested. I am not sure how to catch a DCHECK failure in the test. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:99: // This should not be called before Delegate::OnHeadersSent is invoked, and On 2015/12/15 22:59:40, mef wrote: > OnHeadersSent -> OnHeadersReceived? Done. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:100: // should not be called again unless it IO completes synchronously or until On 2015/12/15 22:59:39, mef wrote: > IO completes -> returns Done. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:117: // stablished, return kProtoUnknown. On 2015/12/15 22:59:40, mef wrote: > established (here and below). Done. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:173: const BidirectionalStreamRequestInfo request_info_; On 2015/12/15 22:59:40, mef wrote: > can this be a scoped_ptr<BidirectionalStreamRequestInfo>? > > If yes, and if it could take an ownership of request_info passed to constructor, > then we would avoid cloning of that structure. Done. I think it should take a raw pointer. Whoever owns this bidi stream should own the request_info, and ensure it is destroyed after bidi stream. I think this is slightly more consistent with SpdyHttpStream. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... File net/http/bidirectional_stream_job.h (right): https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream_job.h:82: RequestPriority priority, On 2015/12/15 22:59:40, mef wrote: > should priority be folded into request_info? Done. I don't see why not. Though in the future we might want to add the ability to set priority after the stream is created. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream_job.h:105: // kProtoSPDYMinimumVersion and kProtoSPDYMaximumVersion. On 2015/12/15 22:59:40, mef wrote: > I suppose that QUIC implementation will return higher version. Done. Good catch! Was copying from SpdyStream, and forgot to change it :\ https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... File net/http/bidirectional_stream_request_info.h (right): https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream_request_info.h:31: bool end_stream_on_headers; On 2015/12/15 22:59:40, mef wrote: > maybe add request_priority field here? Done. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.cc (right): https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:21: const int64 kBufferTimeMs = 1; On 2015/12/15 22:59:40, mef wrote: > would be nice to have comment. Also consider moving into namespace {} as current > code means that it is public symbol net::kBufferTimeMs. Done. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:28: more_read_data_pending_(false), On 2015/12/15 22:59:40, mef wrote: > initialize all fields, e.g. |read_buffer_len_| is not initialized. Done. Good catch! https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:76: if (!stream_closed_) On 2015/12/15 22:59:40, mef wrote: > FWIW it seems that |stream_closed_| is equivalent of !stream_, am I missing > something? Done. I got rid of this check. But |stream_closed| is not exactly equivalent to !stream_, because !stream_ is true when stream_ is not established as well. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:198: DCHECK_EQ(OK, status); On 2015/12/15 23:14:28, mef wrote: > this seems unneeded due to line 191. Done. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.cc:227: delegate_->OnFailed(static_cast<Error>(rv)); On 2015/12/15 23:14:28, mef wrote: > I think delegate_->OnFailed now takes int. Done. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... File net/spdy/bidirectional_stream_spdy_job.h (right): https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:41: RequestPriority priority, On 2015/12/15 22:59:40, mef wrote: > nit: any particular reason why priority is not a part of request_info? Done. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:72: bool more_read_data_pending_; On 2015/12/15 22:59:40, mef wrote: > move close to other read-related fields like data_queue_? Done. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:74: BidirectionalStreamRequestInfo request_info_; On 2015/12/15 22:59:40, mef wrote: > Here is another copy of BidirectionalStreamRequestInfo struct. It would be nice > if original object allocated by the consumer could be passed down to here as > scoped_ptr<BidirectionalStreamRequestInfo>. Done. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:76: SpdyReadQueue data_queue_; On 2015/12/15 22:59:40, mef wrote: > optional nit: maybe call this read_data_queue_? Done. https://codereview.chromium.org/1326503003/diff/930001/net/spdy/bidirectional... net/spdy/bidirectional_stream_spdy_job.h:85: // After |stream_| has been closed, this keeps track of the total number of On 2015/12/15 22:59:40, mef wrote: > move these together with closed_stream_ and closed_stream_status_. Done. Good idea.
https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:92: // |this| should not be destroyed during Delegate::OnHeadersSent or On 2015/12/16 00:26:08, xunjieli wrote: > On 2015/12/15 22:59:40, mef wrote: > > That's an interesting comment. Why not? Do we need tests for this? > > It's illegal to call Cancel during OnHeadersSent or OnDataSent callback. > (See > https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_stre...) > > Not sure how this can be tested. I am not sure how to catch a DCHECK failure in > the test. Acknowledged. This should not be a problem for CronetBidirectionalStream as Destroy is always posted to the network thread and never invoked synchronously. https://codereview.chromium.org/1326503003/diff/930001/net/http/bidirectional... net/http/bidirectional_stream.h:173: const BidirectionalStreamRequestInfo request_info_; On 2015/12/16 00:26:09, xunjieli wrote: > On 2015/12/15 22:59:40, mef wrote: > > can this be a scoped_ptr<BidirectionalStreamRequestInfo>? > > > > If yes, and if it could take an ownership of request_info passed to > constructor, > > then we would avoid cloning of that structure. > > Done. I think it should take a raw pointer. Whoever owns this bidi stream should > own the request_info, and ensure it is destroyed after bidi stream. I think this > is slightly more consistent with SpdyHttpStream. Excellent, sgtm!
A couple comments - sorry for not doing a full pass. I should get to one tomorrow. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:42: : request_info_(request_info), Should either take this as a scoped_ptr and take ownership, or make a copy of it. If we make a copy, argument should be passed as a const reference. Alternatively, make the class have an HttpRequestInfo in it, and do the conversion in the constructor, rather than in Start(). https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:46: timer_(timer.release()) { std::move https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:55: if (!request_info->url.SchemeIsCryptographic()) { I suggest just SchemeIs(url::kHttpsScheme). wss is cryptographic, but we don't support it. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:97: if (stream_request_) If not needed - reset() has the smarts to do nothing if already NULL. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:100: stream_job_->Cancel(); I assume we don't just reset it here so the caller can still poke it for information? Just want to make sure there's a reason. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:161: stream_request_.reset(); nit: May want to move this up a line, before settings stream_job_ https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:13: #include "net/http/bidirectional_stream_request_info.h" Can be forward declared. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:16: #include "url/gurl.h" Can forward declare GURL and SSLConfig. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:33: private HttpStreamRequest::Delegate { private inheritance is banned by the style guide. You can make the overridden methods private, if you want. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:88: // the request. |session| is the http network session with which this request Mention |request_info| must be non-NULL? https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:89: // will be made. |delegate| must be non-NULL. Mention lifetime requirements relative to |session| and |delegate|? https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:116: // invoked. Mention |end_stream| https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:119: // If there is |stream_request_|, cancel it. If |stream_job_| is established, is a |stream_request_| (or if |stream_request_| is non-NULL) https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:124: // Getters that should only be called after Delegate::OnHeadersSent: No longer true https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:184: const BoundNetLog net_log_; include net_log.h https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:190: scoped_ptr<BidirectionalStreamJob> stream_job_; Should document these. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream_unittest.cc:194: class CancelStreamDelegate : public TestDelegateBase { A bit of a pain, but if we allow cancellation in other callbacks, should test them, too (or better, deletion, if we allow it). Contracts around lifetimes seem like the most likely thing to regress across SPDY changes. https://codereview.chromium.org/1326503003/diff/1090001/net/http/http_stream_... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/http_stream_... net/http/http_stream_factory_impl.cc:303: new BidirectionalStreamSpdyJob(spdy_session)); While the code this adds isn't huge, can we prevent it from being included in Chrome binaries? Compilers aren't nearly smart enough to know this is never reached. Maybe an if-def? If not defined, just have this do nothing (Or be a DCHECK) and have RequestBidirectionalStreamJob just have a NOTREACHED? Would also need to exclude the unit tests based on that define.
Patchset #33 (id:1110001) has been deleted
Thanks a lot for the review! I have added tests for deletion of stream, and a compile flag to exclude the sources by default. PTAL. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:42: : request_info_(request_info), On 2015/12/16 23:04:36, mmenke wrote: > Should either take this as a scoped_ptr and take ownership, or make a copy of > it. If we make a copy, argument should be passed as a const reference. > I was following SpdyHttpStream's example. It doesn't take ownership of the request info. Should I take ownership? > Alternatively, make the class have an HttpRequestInfo in it, and do the > conversion in the constructor, rather than in Start(). The HttpRequestInfo doesn't contain RequestPriority, so I think it should still take in a BidirectionalStreamRequestInfo. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:46: timer_(timer.release()) { On 2015/12/16 23:04:36, mmenke wrote: > std::move Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:55: if (!request_info->url.SchemeIsCryptographic()) { On 2015/12/16 23:04:36, mmenke wrote: > I suggest just SchemeIs(url::kHttpsScheme). wss is cryptographic, but we don't > support it. Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:97: if (stream_request_) On 2015/12/16 23:04:36, mmenke wrote: > If not needed - reset() has the smarts to do nothing if already NULL. Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:100: stream_job_->Cancel(); On 2015/12/16 23:04:36, mmenke wrote: > I assume we don't just reset it here so the caller can still poke it for > information? Just want to make sure there's a reason. Done. You are right. I think we should reset the job. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:161: stream_request_.reset(); On 2015/12/16 23:04:36, mmenke wrote: > nit: May want to move this up a line, before settings stream_job_ Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:13: #include "net/http/bidirectional_stream_request_info.h" On 2015/12/16 23:04:36, mmenke wrote: > Can be forward declared. Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:16: #include "url/gurl.h" On 2015/12/16 23:04:36, mmenke wrote: > Can forward declare GURL and SSLConfig. Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:33: private HttpStreamRequest::Delegate { On 2015/12/16 23:04:36, mmenke wrote: > private inheritance is banned by the style guide. You can make the overridden > methods private, if you want. The overridden methods are private. If I use public inheritance, I get "warning C4275: non dll-interface class 'net::BidirectionalStreamJob::Delegate' used as base for dll-interface class 'net::BidirectionalStream'" from win_chromium_compile_dbg_ng bot. Any suggestion? https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:88: // the request. |session| is the http network session with which this request On 2015/12/16 23:04:36, mmenke wrote: > Mention |request_info| must be non-NULL? Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:89: // will be made. |delegate| must be non-NULL. On 2015/12/16 23:04:36, mmenke wrote: > Mention lifetime requirements relative to |session| and |delegate|? Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:116: // invoked. On 2015/12/16 23:04:36, mmenke wrote: > Mention |end_stream| Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:119: // If there is |stream_request_|, cancel it. If |stream_job_| is established, On 2015/12/16 23:04:36, mmenke wrote: > is a |stream_request_| (or if |stream_request_| is non-NULL) Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:124: // Getters that should only be called after Delegate::OnHeadersSent: On 2015/12/16 23:04:36, mmenke wrote: > No longer true Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:184: const BoundNetLog net_log_; On 2015/12/16 23:04:36, mmenke wrote: > include net_log.h I forward declared it. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:190: scoped_ptr<BidirectionalStreamJob> stream_job_; On 2015/12/16 23:04:36, mmenke wrote: > Should document these. Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... File net/http/bidirectional_stream_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream_unittest.cc:194: class CancelStreamDelegate : public TestDelegateBase { On 2015/12/16 23:04:37, mmenke wrote: > A bit of a pain, but if we allow cancellation in other callbacks, should test > them, too (or better, deletion, if we allow it). Contracts around lifetimes > seem like the most likely thing to regress across SPDY changes. Done. Good idea! https://codereview.chromium.org/1326503003/diff/1090001/net/http/http_stream_... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/http_stream_... net/http/http_stream_factory_impl.cc:303: new BidirectionalStreamSpdyJob(spdy_session)); On 2015/12/16 23:04:37, mmenke wrote: > While the code this adds isn't huge, can we prevent it from being included in > Chrome binaries? Compilers aren't nearly smart enough to know this is never > reached. Maybe an if-def? If not defined, just have this do nothing (Or be a > DCHECK) and have RequestBidirectionalStreamJob just have a NOTREACHED? Would > also need to exclude the unit tests based on that define. Done.
https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:184: const BoundNetLog net_log_; On 2015/12/17 18:18:54, xunjieli wrote: > On 2015/12/16 23:04:36, mmenke wrote: > > include net_log.h > > I forward declared it. You can't forward declare types of class member variables. This only compiles without the include because it's including files that include net_log.h. You can only forward declare types if you only deal with pointers/references to them in the header file.
https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:42: : request_info_(request_info), On 2015/12/17 18:18:54, xunjieli wrote: > On 2015/12/16 23:04:36, mmenke wrote: > > Should either take this as a scoped_ptr and take ownership, or make a copy of > > it. If we make a copy, argument should be passed as a const reference. > > > I was following SpdyHttpStream's example. It doesn't take ownership of the > request info. Should I take ownership? > > > Alternatively, make the class have an HttpRequestInfo in it, and do the > > conversion in the constructor, rather than in Start(). > > The HttpRequestInfo doesn't contain RequestPriority, so I think it should still > take in a BidirectionalStreamRequestInfo. SpdyHttpStream is a little different - it's owned (indirectly) by the net object that owns the RequestInfo. This class is more like URLRequest + URLRequestJob...The latter of which owned the RequestInfo. It's best not to rely on the lifetimes of embedder-owned objects. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:33: private HttpStreamRequest::Delegate { On 2015/12/17 18:18:54, xunjieli wrote: > On 2015/12/16 23:04:36, mmenke wrote: > > private inheritance is banned by the style guide. You can make the overridden > > methods private, if you want. > > The overridden methods are private. If I use public inheritance, I get "warning > C4275: non dll-interface class 'net::BidirectionalStreamJob::Delegate' used as > base for dll-interface class 'net::BidirectionalStream'" from > win_chromium_compile_dbg_ng bot. Any suggestion? NON_EXPORTED_BASE(public BidirectionalStreamJob::Delegate), and same for the other. include base/compiler_specific.h.
Thanks, Matt! Much appreciated. PTAL. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.cc:42: : request_info_(request_info), On 2015/12/17 18:58:20, mmenke wrote: > On 2015/12/17 18:18:54, xunjieli wrote: > > On 2015/12/16 23:04:36, mmenke wrote: > > > Should either take this as a scoped_ptr and take ownership, or make a copy > of > > > it. If we make a copy, argument should be passed as a const reference. > > > > > I was following SpdyHttpStream's example. It doesn't take ownership of the > > request info. Should I take ownership? > > > > > Alternatively, make the class have an HttpRequestInfo in it, and do the > > > conversion in the constructor, rather than in Start(). > > > > The HttpRequestInfo doesn't contain RequestPriority, so I think it should > still > > take in a BidirectionalStreamRequestInfo. > > SpdyHttpStream is a little different - it's owned (indirectly) by the net object > that owns the RequestInfo. This class is more like URLRequest + > URLRequestJob...The latter of which owned the RequestInfo. It's best not to > rely on the lifetimes of embedder-owned objects. Done. I see. That makes sense! https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... File net/http/bidirectional_stream.h (right): https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:33: private HttpStreamRequest::Delegate { On 2015/12/17 18:58:20, mmenke wrote: > On 2015/12/17 18:18:54, xunjieli wrote: > > On 2015/12/16 23:04:36, mmenke wrote: > > > private inheritance is banned by the style guide. You can make the > overridden > > > methods private, if you want. > > > > The overridden methods are private. If I use public inheritance, I get > "warning > > C4275: non dll-interface class 'net::BidirectionalStreamJob::Delegate' used as > > base for dll-interface class 'net::BidirectionalStream'" from > > win_chromium_compile_dbg_ng bot. Any suggestion? > > NON_EXPORTED_BASE(public BidirectionalStreamJob::Delegate), and same for the > other. > > include base/compiler_specific.h. Done. https://codereview.chromium.org/1326503003/diff/1090001/net/http/bidirectiona... net/http/bidirectional_stream.h:184: const BoundNetLog net_log_; On 2015/12/17 18:36:29, mmenke wrote: > On 2015/12/17 18:18:54, xunjieli wrote: > > On 2015/12/16 23:04:36, mmenke wrote: > > > include net_log.h > > > > I forward declared it. > > You can't forward declare types of class member variables. This only compiles > without the include because it's including files that include net_log.h. > > You can only forward declare types if you only deal with pointers/references to > them in the header file. Done. Ah, I see. That makes sense. Will keep this distinction in mind.
Patchset #31 (id:1070001) has been deleted
https://codereview.chromium.org/1326503003/diff/1150001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1326503003/diff/1150001/build/common.gypi#new... build/common.gypi:601: 'enable_bidirectional_stream': 0, do we need to explicitly enable it in cronet? https://codereview.chromium.org/1326503003/diff/1150001/net/http/bidirectiona... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/1150001/net/http/bidirectiona... net/http/bidirectional_stream.cc:80: Cancel(); Do we actually need to cancel here? It is a bit weird in case of success. https://codereview.chromium.org/1326503003/diff/1150001/net/http/bidirectiona... net/http/bidirectional_stream.cc:182: delegate_->OnFailed(static_cast<Error>(result)); delegate_->OnFailed() takes an int. https://codereview.chromium.org/1326503003/diff/1150001/net/http/bidirectiona... net/http/bidirectional_stream.cc:192: delegate_->OnFailed(static_cast<Error>(result)); delegate_->OnFailed() takes an int.
Patchset #3 (id:160001) has been deleted
Patchset #28 (id:1030001) has been deleted
Thanks a lot for the review! PTAL. https://codereview.chromium.org/1326503003/diff/1150001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1326503003/diff/1150001/build/common.gypi#new... build/common.gypi:601: 'enable_bidirectional_stream': 0, On 2015/12/17 22:42:09, mef wrote: > do we need to explicitly enable it in cronet? In Cronet, we can use the preprocessor define ENABLE_BIDIRECTIONAL_STREAM. But yes, we will need a follow-up CL to enable this when running net_unittests on Cronet bots. https://codereview.chromium.org/1326503003/diff/1150001/net/http/bidirectiona... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/1150001/net/http/bidirectiona... net/http/bidirectional_stream.cc:80: Cancel(); On 2015/12/17 22:42:09, mef wrote: > Do we actually need to cancel here? It is a bit weird in case of success. In case of success, it should be no-op. Looking at net::URLRequest, it does Cancel() too in the destructor. I added this per mmenke's earlier comment. I think we should clean up in the destructor. https://codereview.chromium.org/1326503003/diff/1150001/net/http/bidirectiona... net/http/bidirectional_stream.cc:182: delegate_->OnFailed(static_cast<Error>(result)); On 2015/12/17 22:42:09, mef wrote: > delegate_->OnFailed() takes an int. Done. https://codereview.chromium.org/1326503003/diff/1150001/net/http/bidirectiona... net/http/bidirectional_stream.cc:192: delegate_->OnFailed(static_cast<Error>(result)); On 2015/12/17 22:42:09, mef wrote: > delegate_->OnFailed() takes an int. Done.
https://codereview.chromium.org/1326503003/diff/1150001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1326503003/diff/1150001/build/common.gypi#new... build/common.gypi:601: 'enable_bidirectional_stream': 0, On 2015/12/18 15:15:23, xunjieli (OOO1223-0118) wrote: > On 2015/12/17 22:42:09, mef wrote: > > do we need to explicitly enable it in cronet? > > In Cronet, we can use the preprocessor define ENABLE_BIDIRECTIONAL_STREAM. But > yes, we will need a follow-up CL to enable this when running net_unittests on > Cronet bots. Hrm, just setting the define ENABLE_BIDIRECTIONAL_STREAM on Cronet project won't help, but it shouldn't be a problem to set in cr_cronet.py and on bots.
https://codereview.chromium.org/1326503003/diff/1150001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1326503003/diff/1150001/build/common.gypi#new... build/common.gypi:601: 'enable_bidirectional_stream': 0, On 2015/12/18 15:45:03, mef wrote: > On 2015/12/18 15:15:23, xunjieli (OOO1223-0118) wrote: > > On 2015/12/17 22:42:09, mef wrote: > > > do we need to explicitly enable it in cronet? > > > > In Cronet, we can use the preprocessor define ENABLE_BIDIRECTIONAL_STREAM. But > > yes, we will need a follow-up CL to enable this when running net_unittests on > > Cronet bots. > > Hrm, just setting the define ENABLE_BIDIRECTIONAL_STREAM on Cronet project won't > help, but it shouldn't be a problem to set in cr_cronet.py and on bots. Ah, yes, you are right! The ENABLE_BIDIRECTIONAL_STREAM needs to be set when compiling net/, so we either need a custom net target, or use the variable when running gyp. We probably also need to have a custom Cronet build too for this purpose. I don't imagine cronet customers of the UrlRequest interface is interested in the BidirectionalStream support.
On 2015/12/18 15:59:29, xunjieli (OOO1223-0118) wrote: > https://codereview.chromium.org/1326503003/diff/1150001/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/1326503003/diff/1150001/build/common.gypi#new... > build/common.gypi:601: 'enable_bidirectional_stream': 0, > On 2015/12/18 15:45:03, mef wrote: > > On 2015/12/18 15:15:23, xunjieli (OOO1223-0118) wrote: > > > On 2015/12/17 22:42:09, mef wrote: > > > > do we need to explicitly enable it in cronet? > > > > > > In Cronet, we can use the preprocessor define ENABLE_BIDIRECTIONAL_STREAM. > But > > > yes, we will need a follow-up CL to enable this when running net_unittests > on > > > Cronet bots. > > > > Hrm, just setting the define ENABLE_BIDIRECTIONAL_STREAM on Cronet project > won't > > help, but it shouldn't be a problem to set in cr_cronet.py and on bots. > > Ah, yes, you are right! The ENABLE_BIDIRECTIONAL_STREAM needs to be set when > compiling net/, so we either need a custom net target, or use the variable when > running gyp. We probably also need to have a custom Cronet build too for this > purpose. I don't imagine cronet customers of the UrlRequest interface is > interested in the BidirectionalStream support. I would rather not create multiple ever-so-slightly different versions of Cronet, but builds of Cronet on regular Android bots share net/ with Chromium, so they will have to exclude the bidirectional stream from Cronet. We should consider removing Cronet targets from regular Chrome/android build, but that's not related to this CL. I'm trying to rebase and test my CL on top of your latest changes...
On 2015/12/18 17:22:01, mef wrote: > On 2015/12/18 15:59:29, xunjieli (OOO1223-0118) wrote: > > https://codereview.chromium.org/1326503003/diff/1150001/build/common.gypi > > File build/common.gypi (right): > > > > > https://codereview.chromium.org/1326503003/diff/1150001/build/common.gypi#new... > > build/common.gypi:601: 'enable_bidirectional_stream': 0, > > On 2015/12/18 15:45:03, mef wrote: > > > On 2015/12/18 15:15:23, xunjieli (OOO1223-0118) wrote: > > > > On 2015/12/17 22:42:09, mef wrote: > > > > > do we need to explicitly enable it in cronet? > > > > > > > > In Cronet, we can use the preprocessor define ENABLE_BIDIRECTIONAL_STREAM. > > But > > > > yes, we will need a follow-up CL to enable this when running net_unittests > > on > > > > Cronet bots. > > > > > > Hrm, just setting the define ENABLE_BIDIRECTIONAL_STREAM on Cronet project > > won't > > > help, but it shouldn't be a problem to set in cr_cronet.py and on bots. > > > > Ah, yes, you are right! The ENABLE_BIDIRECTIONAL_STREAM needs to be set when > > compiling net/, so we either need a custom net target, or use the variable > when > > running gyp. We probably also need to have a custom Cronet build too for this > > purpose. I don't imagine cronet customers of the UrlRequest interface is > > interested in the BidirectionalStream support. > > I would rather not create multiple ever-so-slightly different versions of > Cronet, but builds of Cronet on regular Android bots share net/ with Chromium, > so they will have to exclude the bidirectional stream from Cronet. > > We should consider removing Cronet targets from regular Chrome/android build, > but that's not related to this CL. > > I'm trying to rebase and test my CL on top of your latest changes... I've verified that CronetBidirectionalStream impelementation builds and passes tests with enable_bidirectional_stream=1 gyp variable set. This lgtm, but please wait for Matt's approval. You will also need OWNER approval for build/common.gypi
xunjieli@chromium.org changed reviewers: + jbudorick@chromium.org
jbudorick@, could you take a look at build/common.gyp? Thanks!
xunjieli@chromium.org changed reviewers: - jbudorick@chromium.org, rch@chromium.org
xunjieli@chromium.org changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/1326503003/diff/1190001/net/http/bidirectiona... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/1190001/net/http/bidirectiona... net/http/bidirectional_stream.cc:165: stream_job_->Start(request_info_.get(), net_log_, this, timer_.Pass()); please use std::move() https://codereview.chromium.org/1326503003/diff/1190001/net/http/http_stream_... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1326503003/diff/1190001/net/http/http_stream_... net/http/http_stream_factory_impl_job.cc:410: bidirectional_stream_job_.Pass(), please use std::move() https://codereview.chromium.org/1326503003/diff/1190001/net/http/http_stream_... net/http/http_stream_factory_impl_job.cc:415: request_->OnNewSpdySessionReady(this, stream_.Pass(), please use std::move()
On 2015/12/18 18:09:23, xunjieli (OOO1223-0118) wrote: > jbudorick@, could you take a look at build/common.gyp? Thanks! build/common.gypi lgtm
On 2015/12/18 21:44:22, jbudorick wrote: > On 2015/12/18 18:09:23, xunjieli (OOO1223-0118) wrote: > > jbudorick@, could you take a look at build/common.gyp? Thanks! > > build/common.gypi lgtm Got sidetracked this week. :( I'll get back to it after the break (Or during it, in the unlikely case I'm feeling ambitious).
> Got sidetracked this week. :( I'll get back to it after the break (Or during > it, in the unlikely case I'm feeling ambitious). No problem! Please take a nice break, and review this only when you get back. You only get a chance once a year to enjoy this holiday :) https://codereview.chromium.org/1326503003/diff/1190001/net/http/bidirectiona... File net/http/bidirectional_stream.cc (right): https://codereview.chromium.org/1326503003/diff/1190001/net/http/bidirectiona... net/http/bidirectional_stream.cc:165: stream_job_->Start(request_info_.get(), net_log_, this, timer_.Pass()); On 2015/12/18 21:40:00, mef wrote: > please use std::move() Done. Good catch! https://codereview.chromium.org/1326503003/diff/1190001/net/http/http_stream_... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1326503003/diff/1190001/net/http/http_stream_... net/http/http_stream_factory_impl_job.cc:410: bidirectional_stream_job_.Pass(), On 2015/12/18 21:40:00, mef wrote: > please use std::move() Done. https://codereview.chromium.org/1326503003/diff/1190001/net/http/http_stream_... net/http/http_stream_factory_impl_job.cc:415: request_->OnNewSpdySessionReady(this, stream_.Pass(), On 2015/12/18 21:40:00, mef wrote: > please use std::move() Done.
https://codereview.chromium.org/1326503003/diff/1210001/net/net.gypi File net/net.gypi (right): https://codereview.chromium.org/1326503003/diff/1210001/net/net.gypi#newcode136 net/net.gypi:136: 'http/bidirectional_stream_job.cc', any particular reason why bidirectional_stream_job.* can't go under net_bidirectional_stream_sources?
https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_network... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_network... net/http/http_network_transaction.h:95: BidirectionalStreamJob* stream) override; forward-declare BidirectionalStreamJob? https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... net/http/http_stream_factory_impl_job.cc:29: #include "net/http/bidirectional_stream_job.h" Move into conditional include? https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... net/http/http_stream_factory_impl_job.cc:370: DCHECK(bidirectional_stream_job_); Should this and other sections be under #if defined(ENABLE_BIDIRECTIONAL_STREAM)? https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... net/http/http_stream_factory_impl_job.h:376: scoped_ptr<BidirectionalStreamJob> bidirectional_stream_job_; Forward-declare BidirectionalStreamJob? https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... net/http/http_stream_factory_impl_request.h:73: scoped_ptr<BidirectionalStreamJob> bidirectional_stream_spdy_job, Forward-declare BidirectionalStreamJob? https://codereview.chromium.org/1326503003/diff/1210001/net/spdy/spdy_test_ut... File net/spdy/spdy_test_util_common.cc (right): https://codereview.chromium.org/1326503003/diff/1210001/net/spdy/spdy_test_ut... net/spdy/spdy_test_util_common.cc:1236: return ConstructSpdyGetSynReply(extra_headers, extra_header_count, 1); why this changed?
On 2015/12/21 17:31:43, mef wrote: > https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_network... > File net/http/http_network_transaction.h (right): > > https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_network... > net/http/http_network_transaction.h:95: BidirectionalStreamJob* stream) > override; > forward-declare BidirectionalStreamJob? > > https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... > File net/http/http_stream_factory_impl_job.cc (right): > > https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... > net/http/http_stream_factory_impl_job.cc:29: #include > "net/http/bidirectional_stream_job.h" > Move into conditional include? > > https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... > net/http/http_stream_factory_impl_job.cc:370: DCHECK(bidirectional_stream_job_); > Should this and other sections be under #if > defined(ENABLE_BIDIRECTIONAL_STREAM)? > > https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... > File net/http/http_stream_factory_impl_job.h (right): > > https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... > net/http/http_stream_factory_impl_job.h:376: scoped_ptr<BidirectionalStreamJob> > bidirectional_stream_job_; > Forward-declare BidirectionalStreamJob? > > https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... > File net/http/http_stream_factory_impl_request.h (right): > > https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... > net/http/http_stream_factory_impl_request.h:73: > scoped_ptr<BidirectionalStreamJob> bidirectional_stream_spdy_job, > Forward-declare BidirectionalStreamJob? > > https://codereview.chromium.org/1326503003/diff/1210001/net/spdy/spdy_test_ut... > File net/spdy/spdy_test_util_common.cc (right): > > https://codereview.chromium.org/1326503003/diff/1210001/net/spdy/spdy_test_ut... > net/spdy/spdy_test_util_common.cc:1236: return > ConstructSpdyGetSynReply(extra_headers, extra_header_count, 1); > why this changed? Also, need to rebase on master, as SequencedSocketData no longer has CompleteRead() method.
Sorry it took me some time to fix the tests. Didn't know about Matt's change on SequencedSocketData.. PTAL. https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_network... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_network... net/http/http_network_transaction.h:95: BidirectionalStreamJob* stream) override; On 2015/12/21 17:31:42, mef wrote: > forward-declare BidirectionalStreamJob? Done. https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... net/http/http_stream_factory_impl_job.cc:29: #include "net/http/bidirectional_stream_job.h" On 2015/12/21 17:31:43, mef wrote: > Move into conditional include? Done. https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... net/http/http_stream_factory_impl_job.cc:370: DCHECK(bidirectional_stream_job_); On 2015/12/21 17:31:42, mef wrote: > Should this and other sections be under #if > defined(ENABLE_BIDIRECTIONAL_STREAM)? Done. https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... net/http/http_stream_factory_impl_job.h:376: scoped_ptr<BidirectionalStreamJob> bidirectional_stream_job_; On 2015/12/21 17:31:43, mef wrote: > Forward-declare BidirectionalStreamJob? Done. https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... File net/http/http_stream_factory_impl_request.h (right): https://codereview.chromium.org/1326503003/diff/1210001/net/http/http_stream_... net/http/http_stream_factory_impl_request.h:73: scoped_ptr<BidirectionalStreamJob> bidirectional_stream_spdy_job, On 2015/12/21 17:31:43, mef wrote: > Forward-declare BidirectionalStreamJob? Done. https://codereview.chromium.org/1326503003/diff/1210001/net/net.gypi File net/net.gypi (right): https://codereview.chromium.org/1326503003/diff/1210001/net/net.gypi#newcode136 net/net.gypi:136: 'http/bidirectional_stream_job.cc', On 2015/12/21 17:09:09, mef wrote: > any particular reason why bidirectional_stream_job.* can't go under > net_bidirectional_stream_sources? Done. https://codereview.chromium.org/1326503003/diff/1210001/net/spdy/spdy_test_ut... File net/spdy/spdy_test_util_common.cc (right): https://codereview.chromium.org/1326503003/diff/1210001/net/spdy/spdy_test_ut... net/spdy/spdy_test_util_common.cc:1236: return ConstructSpdyGetSynReply(extra_headers, extra_header_count, 1); On 2015/12/21 17:31:43, mef wrote: > why this changed? I was trying to test setting extra headers on the HEADER block, as recommended by Bence. The method wasn't passing |extra_headers| argument correctly. https://codereview.chromium.org/1326503003/diff/1230001/net/http/http_stream_... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/1326503003/diff/1230001/net/http/http_stream_... net/http/http_stream_factory.h:18: #include "net/http/bidirectional_stream_job.h" Had to add this because otherwise when HttpStreamFactoryImpl job import this file, it complains that it cannot determine the size of the forward declared BidirectionalStreamJob class. WebSocket did the same on line 25, even though the file is included in websocket sources.
still lgtm with nits. https://codereview.chromium.org/1326503003/diff/1210001/net/spdy/spdy_test_ut... File net/spdy/spdy_test_util_common.cc (right): https://codereview.chromium.org/1326503003/diff/1210001/net/spdy/spdy_test_ut... net/spdy/spdy_test_util_common.cc:1236: return ConstructSpdyGetSynReply(extra_headers, extra_header_count, 1); On 2015/12/21 22:01:22, xunjieli (OOO1223-0118) wrote: > On 2015/12/21 17:31:43, mef wrote: > > why this changed? > > I was trying to test setting extra headers on the HEADER block, as recommended > by Bence. The method wasn't passing |extra_headers| argument correctly. Acknowledged. https://codereview.chromium.org/1326503003/diff/1230001/net/http/http_stream_... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/1326503003/diff/1230001/net/http/http_stream_... net/http/http_stream_factory.h:18: #include "net/http/bidirectional_stream_job.h" On 2015/12/21 22:01:22, xunjieli (OOO1223-0118) wrote: > Had to add this because otherwise when HttpStreamFactoryImpl job import this > file, it complains that it cannot determine the size of the forward declared > BidirectionalStreamJob class. > > WebSocket did the same on line 25, even though the file is included in websocket > sources. Yeah, I'm not sure why this is happening. It also compiles if I forward-declare it here and unconditionally #include "net/http/bidirectional_stream_job.h" into http_stream_factory_impl_job.cc. https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_network... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_network... net/http/http_network_transaction.cc:530: BidirectionalStreamJob* stream) { nit: I believe parameter name |stream| should match class name, so I suggest naming it |stream_job|. https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_network... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_network... net/http/http_network_transaction.h:96: BidirectionalStreamJob* stream) override; nit: I believe parameter name |stream| should match class name, so I suggest naming it |stream_job|. https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_stream_... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_stream_... net/http/http_stream_factory_impl_request.cc:95: BidirectionalStreamJob* stream) { stream -> stream_job? https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_stream_... File net/http/http_stream_factory_impl_request_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_stream_... net/http/http_stream_factory_impl_request_unittest.cc:40: BidirectionalStreamJob* stream) override {} stream -> stream_job?
Thanks! Talked with Misha offline. I think I am going to land this, since the sources will only get compiled behind a flag. I am going to be out for 3 weeks, and Misha's work on Cronet layer is blocked on this. I am going to enable the flag on Cronet bots, and Misha can work on top of this and help with iterating. https://codereview.chromium.org/1326503003/diff/1230001/net/http/http_stream_... File net/http/http_stream_factory.h (right): https://codereview.chromium.org/1326503003/diff/1230001/net/http/http_stream_... net/http/http_stream_factory.h:18: #include "net/http/bidirectional_stream_job.h" On 2015/12/21 23:21:31, mef wrote: > On 2015/12/21 22:01:22, xunjieli (OOO1223-0118) wrote: > > Had to add this because otherwise when HttpStreamFactoryImpl job import this > > file, it complains that it cannot determine the size of the forward declared > > BidirectionalStreamJob class. > > > > WebSocket did the same on line 25, even though the file is included in > websocket > > sources. > > Yeah, I'm not sure why this is happening. It also compiles if I forward-declare > it here and unconditionally #include "net/http/bidirectional_stream_job.h" into > http_stream_factory_impl_job.cc. > Acknowledged. https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_network... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_network... net/http/http_network_transaction.cc:530: BidirectionalStreamJob* stream) { On 2015/12/21 23:21:31, mef wrote: > nit: I believe parameter name |stream| should match class name, so I suggest > naming it |stream_job|. Done. https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_network... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_network... net/http/http_network_transaction.h:96: BidirectionalStreamJob* stream) override; On 2015/12/21 23:21:31, mef wrote: > nit: I believe parameter name |stream| should match class name, so I suggest > naming it |stream_job|. Done. https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_stream_... File net/http/http_stream_factory_impl_request.cc (right): https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_stream_... net/http/http_stream_factory_impl_request.cc:95: BidirectionalStreamJob* stream) { On 2015/12/21 23:21:31, mef wrote: > stream -> stream_job? Done. https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_stream_... File net/http/http_stream_factory_impl_request_unittest.cc (right): https://codereview.chromium.org/1326503003/diff/1250001/net/http/http_stream_... net/http/http_stream_factory_impl_request_unittest.cc:40: BidirectionalStreamJob* stream) override {} On 2015/12/21 23:21:31, mef wrote: > stream -> stream_job? Done.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, mef@chromium.org Link to the patchset: https://codereview.chromium.org/1326503003/#ps1270001 (title: "Address Misha's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1326503003/1270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1326503003/1270001
Message was sent while issue was closed.
Description was changed from ========== Added a net::BidirectionalStream to expose a bidirectional streaming interface This CL adds a BidirectionalStream on top of SpdyStream to expose a bidirectional streaming interface. This CL also modifies HttpStreamFactoryImplJob to create BidirectionalStream. BUG=516342 ========== to ========== Added a net::BidirectionalStream to expose a bidirectional streaming interface This CL adds a BidirectionalStream on top of SpdyStream to expose a bidirectional streaming interface. This CL also modifies HttpStreamFactoryImplJob to create BidirectionalStream. BUG=516342 ==========
Message was sent while issue was closed.
Committed patchset #37 (id:1270001)
Message was sent while issue was closed.
Description was changed from ========== Added a net::BidirectionalStream to expose a bidirectional streaming interface This CL adds a BidirectionalStream on top of SpdyStream to expose a bidirectional streaming interface. This CL also modifies HttpStreamFactoryImplJob to create BidirectionalStream. BUG=516342 ========== to ========== Added a net::BidirectionalStream to expose a bidirectional streaming interface This CL adds a BidirectionalStream on top of SpdyStream to expose a bidirectional streaming interface. This CL also modifies HttpStreamFactoryImplJob to create BidirectionalStream. BUG=516342 Committed: https://crrev.com/11834f05b42d7c5e8cc7bd5774033db3773627a5 Cr-Commit-Position: refs/heads/master@{#366541} ==========
Message was sent while issue was closed.
Patchset 37 (id:??) landed as https://crrev.com/11834f05b42d7c5e8cc7bd5774033db3773627a5 Cr-Commit-Position: refs/heads/master@{#366541}
Message was sent while issue was closed.
On 2015/12/21 22:01:22, xunjieli (OOO1223-0118) wrote: > Sorry it took me some time to fix the tests. Didn't know about Matt's change on > SequencedSocketData.. PTAL. Sorry about that, would have mentioned it, but thought you were off for the next two weeks, too (Just drawing down on emails, at the moment - don't like going through two hundred when I get back).
Message was sent while issue was closed.
On 2015/12/22 16:33:37, mmenke (OOO Dec 19 to Jan 3) wrote: > On 2015/12/21 22:01:22, xunjieli (OOO1223-0118) wrote: > > Sorry it took me some time to fix the tests. Didn't know about Matt's change > on > > SequencedSocketData.. PTAL. > > Sorry about that, would have mentioned it, but thought you were off for the next > two weeks, too (Just drawing down on emails, at the moment - don't like going > through two hundred when I get back). Matt, sorry for not waiting for your approval, but Helen is out for a month or so, and I solemnly swear to address all your comments in a followup CL.
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1326503003/diff/1270001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1326503003/diff/1270001/build/common.gypi#new... build/common.gypi:608: # Enables BidrectionalSteam; disabled by default. This is just a new class, why is this behind a build setting? Also, have you seen "[chromium-dev] Intent to implement: New feature flags system for the bulid"? We don't do these global settings any more. (Also, typo BidrectionalSteam)
Message was sent while issue was closed.
https://codereview.chromium.org/1326503003/diff/1270001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1326503003/diff/1270001/build/common.gypi#new... build/common.gypi:608: # Enables BidrectionalSteam; disabled by default. On 2015/12/22 19:12:35, Nico wrote: > This is just a new class, why is this behind a build setting? It is needed to expose bidirectional streaming API from cronet for grpc support. It is not currently used in chrome proper. > > Also, have you seen "[chromium-dev] Intent to implement: New feature flags > system for the bulid"? We don't do these global settings any more. Sorry, we've missed this. I've entered https://code.google.com/p/chromium/issues/detail?id=571696 and will follow an example in https://codereview.chromium.org/1487873003/ > > (Also, typo BidrectionalSteam) Will fix.
Message was sent while issue was closed.
rch@chromium.org changed reviewers: + rch@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1326503003/diff/1270001/net/http/http_stream_... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1326503003/diff/1270001/net/http/http_stream_... net/http/http_stream_factory_impl.cc:21: #if defined(ENABLE_BIDIRECTIONAL_STREAM) drive-by comment: Instead of doing #ifdefs, would it be possible to make this run-time enabled? This would be handy because it would mean that this bi-di code would be compiled when normal builds run. So future refactors of this class would be easier. It also would allow the code to be indexed in code search.
Message was sent while issue was closed.
On 2016/01/08 21:36:33, Ryan Hamilton wrote: > https://codereview.chromium.org/1326503003/diff/1270001/net/http/http_stream_... > File net/http/http_stream_factory_impl.cc (right): > > https://codereview.chromium.org/1326503003/diff/1270001/net/http/http_stream_... > net/http/http_stream_factory_impl.cc:21: #if > defined(ENABLE_BIDIRECTIONAL_STREAM) > drive-by comment: Instead of doing #ifdefs, would it be possible to make this > run-time enabled? This would be handy because it would mean that this bi-di code > would be compiled when normal builds run. So future refactors of this class > would be easier. It also would allow the code to be indexed in code search. Hrm, this was Matt's suggestion to keep bidirectional_stream out of chrome proper, where it would not be used. |
