Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2383533003:
Blimp: define HeliumTransport/HeliumStream interfaces. (Closed)
|
Created:
4 years, 2 months ago by Kevin M Modified:
4 years, 2 months ago CC:
anandc+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, cbentzel+watch_chromium.org, chromium-reviews, dtrainor+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, steimel+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlimp: define HeliumTransport/HeliumStream interfaces.
These classes encapsulate transport-specific code for connecting
logical bidirectional message streams, and the logic for sending
and receiving messages on those streams.
R=scf@chromium.org,perumaal@chromium.org,gcasto@chromium.org
CC=lethalantidote@chromium.org,steimel@chromium.org
BUG=651504
Committed: https://crrev.com/e97d5665eb5f7e7680798a8a0c59fdb8d10817a7
Cr-Commit-Position: refs/heads/master@{#424870}
Patch Set 1 #
Total comments: 7
Patch Set 2 : added ReadMessage comment re: flow control #
Total comments: 8
Patch Set 3 : gcasto feedback #Patch Set 4 : gcasto feedback #Patch Set 5 : Asymmetric connect/listen API #
Total comments: 8
Patch Set 6 : ReadMessage delegate #
Total comments: 3
Patch Set 7 : typo #
Total comments: 13
Patch Set 8 : wez feedback #Patch Set 9 : Back to ReceiveMessage #
Total comments: 8
Patch Set 10 : code review feedback #
Messages
Total messages: 74 (20 generated)
gcasto, perumaal - this is relevant to your interests, care to take a look?
bgoldman@chromium.org changed reviewers: + bgoldman@chromium.org
https://codereview.chromium.org/2383533003/diff/1/blimp/net/helium/helium_str... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/1/blimp/net/helium/helium_str... blimp/net/helium/helium_stream.h:49: // ReceiveMessage() call is made at a time. Maybe I'm just not familiar with the pattern, but it took me a solid one minute of thinking to understand what's going on here. I think this documentation comment could be clearer by saying what the caller is doing/expecting when this method is called. My best shot at it: "Invokes a callback when a HeliumMessage is available to be read from the stream." Also, what's the reason for using this kind of interface, where the caller having to make sure there's only one pending request? I'm a bit worried about "undefined" behavior with that kind of thing. Could you instead have some way to just register a callback that gets invoked repeatedly? I haven't thought a lot about the exact use cases, but if there's a 1:1 relationship between caller object and HeliumStream instance, that might be simpler.
Having a callback that pulls messages out as soon as they're ready means that we lose a valuable pushback signal for read flow control. At some level, we are going to have to ensure that read and write requests are serialized. Since the only user of this class is the SyncManager, I think it's OK to have the interface be a little more cumbersome if it gives the caller finer-grained control over read scheduling. We could have the SyncManager toggle a clear-to-send signal back and forth, but IMO simply using a callback is a much more direct way of communicating to the stream "I'm finished applying the state mutation; give me another packet".
WRT undefined behavior on concurrent reads/writes: gRPC and sockets will throw error codes should this occur, so the constraint is enforced pretty loudly.
https://codereview.chromium.org/2383533003/diff/1/blimp/net/helium/helium_str... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/1/blimp/net/helium/helium_str... blimp/net/helium/helium_stream.h:49: // ReceiveMessage() call is made at a time. On 2016/09/29 18:20:21, Brian Goldman wrote: > Maybe I'm just not familiar with the pattern, but it took me a solid one minute > of thinking to understand what's going on here. > > I think this documentation comment could be clearer by saying what the caller is > doing/expecting when this method is called. My best shot at it: "Invokes a > callback when a HeliumMessage is available to be read from the stream." > > Also, what's the reason for using this kind of interface, where the caller > having to make sure there's only one pending request? I'm a bit worried about > "undefined" behavior with that kind of thing. Could you instead have some way to > just register a callback that gets invoked repeatedly? I haven't thought a lot > about the exact use cases, but if there's a 1:1 relationship between caller > object and HeliumStream instance, that might be simpler. I agree. Or what we could do is make the impl of HeliumStream "bark" if its called two times. https://codereview.chromium.org/2383533003/diff/1/blimp/net/helium/helium_str... blimp/net/helium/helium_stream.h:55: void AddObserver(Observer* observer); dumb q: What is the rationale for allowing multiple observers but just one caller to ReceiveMessage()? https://codereview.chromium.org/2383533003/diff/1/blimp/net/helium/helium_tra... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/1/blimp/net/helium/helium_tra... blimp/net/helium/helium_transport.h:24: virtual void Connect(const ConnectCallback& connect_cb) = 0; does it allow many calls to the Connect on the same instance?
https://codereview.chromium.org/2383533003/diff/1/blimp/net/helium/helium_tra... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/1/blimp/net/helium/helium_tra... blimp/net/helium/helium_transport.h:16: class HeliumTransport { Just to make sure. Would this initially wrap a BlimpTransport internally?
On 2016/09/29 18:29:20, Kevin M wrote: > Having a callback that pulls messages out as soon as they're ready means that we > lose a valuable pushback signal for read flow control. > > At some level, we are going to have to ensure that read and write requests are > serialized. Since the only user of this class is the SyncManager, I think it's > OK to have the interface be a little more cumbersome if it gives the caller > finer-grained control over read scheduling. > > We could have the SyncManager toggle a clear-to-send signal back and forth, but > IMO simply using a callback is a much more direct way of communicating to the > stream "I'm finished applying the state mutation; give me another packet". That makes sense re flow control. I think that's worth putting the documentation for this method.
Added a comment re: flow control. https://codereview.chromium.org/2383533003/diff/1/blimp/net/helium/helium_tra... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/1/blimp/net/helium/helium_tra... blimp/net/helium/helium_transport.h:16: class HeliumTransport { On 2016/09/29 19:13:50, perumaal wrote: > Just to make sure. Would this initially wrap a BlimpTransport internally? That's a possibility, then we'd have a HeliumStream wrap a MessagePort. https://codereview.chromium.org/2383533003/diff/1/blimp/net/helium/helium_tra... blimp/net/helium/helium_transport.h:24: virtual void Connect(const ConnectCallback& connect_cb) = 0; On 2016/09/29 19:12:22, scf wrote: > does it allow many calls to the Connect on the same instance? Yes; there is no FIFO ordering constraint like there is for reads and writes.
https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... blimp/net/helium/helium_stream.h:45: const net::CompletionCallback& callback) = 0; Nit: We may want to define our own callback since we aren't going to use the same error space as //net and I don't think that we care about returning the size of the data written so we can use an enum directly instead having to cast from an integer. https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... blimp/net/helium/helium_transport.h:16: class HeliumTransport { Add OnStreamAccepted()?
https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... blimp/net/helium/helium_stream.h:53: base::Callback<void(std::unique_ptr<HeliumMessage>)> on_receive_cb) = 0; Also, we are eventually going to want to return a status as well as just the HeliumMessage here.
https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... blimp/net/helium/helium_stream.h:45: const net::CompletionCallback& callback) = 0; On 2016/09/30 16:58:42, Garrett Casto wrote: > Nit: We may want to define our own callback since we aren't going to use the > same error space as //net and I don't think that we care about returning the > size of the data written so we can use an enum directly instead having to cast > from an integer. Done. https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... blimp/net/helium/helium_stream.h:53: base::Callback<void(std::unique_ptr<HeliumMessage>)> on_receive_cb) = 0; On 2016/09/30 17:46:20, Garrett Casto wrote: > Also, we are eventually going to want to return a status as well as just the > HeliumMessage here. Done. https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... blimp/net/helium/helium_transport.h:16: class HeliumTransport { On 2016/09/30 16:58:42, Garrett Casto wrote: > Add OnStreamAccepted()? I was thinking that |connect_cb| would suffice for that?
Related: I'm working on a CL which adds a home for canonical error codes.
Description was changed from ========== Blimp: define HeliumTransport/HeliumStream interfaces. These classes encapsulate transport-specific code for connecting logical bidirectional message streams, and the logic for sending and receiving messages on those streams. R=scf@chromium.org,perumaal@chromium.org,gcasto@chromium.org CC=lethalantidote@chromium.org,steimel@chromium.org BUG=651504 ========== to ========== Blimp: define HeliumTransport/HeliumStream interfaces. These classes encapsulate transport-specific code for connecting logical bidirectional message streams, and the logic for sending and receiving messages on those streams. R=scf@chromium.org,perumaal@chromium.org,gcasto@chromium.org CC=lethalantidote@chromium.org,steimel@chromium.org BUG=651504 ==========
kmarshall@chromium.org changed reviewers: + wez@chromium.org
+wez
lethalantidote@chromium.org changed reviewers: + lethalantidote@chromium.org
lgtm
Sorry, wrote this a few days ago, but just realized I didn't actually send the comments. https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... blimp/net/helium/helium_transport.h:16: class HeliumTransport { On 2016/09/30 22:20:56, Kevin M wrote: > On 2016/09/30 16:58:42, Garrett Casto wrote: > > Add OnStreamAccepted()? > > I was thinking that |connect_cb| would suffice for that? There should be a distinction between "I want to create a new HeliumStream", and you get a Stream which you can send data on, and "The peer has created a stream that you should get". You want to be able to distinguish between these two cases so you know if you should be sending first or reading first. Also, I think that the receiving a stream should probably be a delegate/observer since I don't think there is ever a time when we don't want to be notified that a new stream is here.
https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... blimp/net/helium/helium_transport.h:16: class HeliumTransport { On 2016/10/03 22:06:00, Garrett Casto wrote: > On 2016/09/30 22:20:56, Kevin M wrote: > > On 2016/09/30 16:58:42, Garrett Casto wrote: > > > Add OnStreamAccepted()? > > > > I was thinking that |connect_cb| would suffice for that? > > There should be a distinction between "I want to create a new HeliumStream", and > you get a Stream which you can send data on, and "The peer has created a stream > that you should get". You want to be able to distinguish between these two cases > so you know if you should be sending first or reading first. > > Also, I think that the receiving a stream should probably be a delegate/observer > since I don't think there is ever a time when we don't want to be notified that > a new stream is here. Agreed that we'll want a delegate because new incoming streams can be connected without the reciepient having called Connect. I'm not sure if it matters whether or not the stream was initiated by the local side or by the peer, though, since we are allowing a designated part of the sync manager (Engine?) to migrate objects across streams and handle the subsequent message reordering.
On 2016/10/04 01:33:08, Kevin M wrote: > https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... > File blimp/net/helium/helium_transport.h (right): > > https://codereview.chromium.org/2383533003/diff/20001/blimp/net/helium/helium... > blimp/net/helium/helium_transport.h:16: class HeliumTransport { > On 2016/10/03 22:06:00, Garrett Casto wrote: > > On 2016/09/30 22:20:56, Kevin M wrote: > > > On 2016/09/30 16:58:42, Garrett Casto wrote: > > > > Add OnStreamAccepted()? > > > > > > I was thinking that |connect_cb| would suffice for that? > > > > There should be a distinction between "I want to create a new HeliumStream", > and > > you get a Stream which you can send data on, and "The peer has created a > stream > > that you should get". You want to be able to distinguish between these two > cases > > so you know if you should be sending first or reading first. > > > > Also, I think that the receiving a stream should probably be a > delegate/observer > > since I don't think there is ever a time when we don't want to be notified > that > > a new stream is here. > > Agreed that we'll want a delegate because new incoming streams can be connected > without the reciepient having called Connect. I'm not sure if it matters whether > or not the stream was initiated by the local side or by the peer, though, since > we are allowing a designated part of the sync manager (Engine?) to migrate > objects across streams and handle the subsequent message reordering. I'm not sure if we've completely agreed on the exact semantics of how the sync layer handles assignment, but the general case we've talked about so far is that the peer creating the stream chooses which Object is associated with the stream. In that case, you want to know if the Stream you are getting is something that you have requested via Connect(), and thus you should choose which HeliumObject is associated with it and start sending data, or if this was created by your peer and you should read and process data from the Stream first.
Switched to using separate connect/accept calls, PTAL. I'm on the fence about using an Accept w/callback instead of a delegate, but using callbacks over delegates is consistent with how the rest of the Stream and Transport APIs are structured, plus it empowers the Sync layer to dictate when streams should be acquired.
LGTM https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... blimp/net/helium/helium_transport.h:42: // Multiple overlapping Accept requests are permitted. Is there any reason why we would ever want to have overlapping Accept() call outstanding, or is this just trying to give possible flexibility to Sync? I agree that it's a tough call between a Delegate (which is consistent with how we want this call to be used) and Callbacks (which is how the rest of the API is structured). I'm fine with this for now, and as we start writing code we can see what issues we run into.
https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... blimp/net/helium/helium_stream.h:54: virtual void ReceiveMessage( I think this would be better suited as SetMessageReadCallback(...) *once* and not to 'pull' messages repeatedly. If you really want to 'pull' message (i.e. force the engine to send you message), then I would recommend SendMessage'ing of some sort and waiting for the callback. Also, based on the current ongoing implementation of grpc stack, the pull wouldn't quite work because it's a callback when the Stream has a message not when you call ReceiveMessage. https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... blimp/net/helium/helium_transport.h:43: virtual void Accept(const StreamCreatedCallback& cb) = 0; There is no Accept in grpc land. Could we just have the Connect be SetOnConnectionAccepted(...) callback and merge both?
https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... blimp/net/helium/helium_stream.h:54: virtual void ReceiveMessage( On 2016/10/05 20:40:44, perumaal wrote: > I think this would be better suited as SetMessageReadCallback(...) *once* and > not to 'pull' messages repeatedly. > If you really want to 'pull' message (i.e. force the engine to send you > message), then I would recommend SendMessage'ing of some sort and waiting for > the callback. > > Also, based on the current ongoing implementation of grpc stack, the pull > wouldn't quite work because it's a callback when the Stream has a message not > when you call ReceiveMessage. > > Note that we originally had this as a callback that would always be called when you a message was received. We changed this to allow for flow control on the recievers side. However, after a conversation I had with Matt earlier this week, it's possible we may want to change this back. Let me add the two of you to the thread on the design doc discussing this. https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... blimp/net/helium/helium_transport.h:43: virtual void Accept(const StreamCreatedCallback& cb) = 0; On 2016/10/05 20:40:44, perumaal wrote: > There is no Accept in grpc land. Could we just have the Connect be > SetOnConnectionAccepted(...) callback and merge both? Take a look at go/helium-transport. Short answer is that we want to be able to distinguish between initiating a connection and getting a connection that the other side has initiated. gRPC doesn't do this directly, but we will do it internally by having gRPC streams we create to service HeliumTransport::Connect() on the server.
https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... blimp/net/helium/helium_transport.h:43: virtual void Accept(const StreamCreatedCallback& cb) = 0; On 2016/10/05 at 20:53:07, Garrett Casto wrote: > On 2016/10/05 20:40:44, perumaal wrote: > > There is no Accept in grpc land. Could we just have the Connect be > > SetOnConnectionAccepted(...) callback and merge both? > > Take a look at go/helium-transport. Short answer is that we want to be able to distinguish between initiating a connection and getting a connection that the other side has initiated. gRPC doesn't do this directly, but we will do it internally by having gRPC streams we create to service HeliumTransport::Connect() on the server. Discussed offline, this is ok for now. thanks!
https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... blimp/net/helium/helium_transport.h:42: // Multiple overlapping Accept requests are permitted. On 2016/10/05 20:34:05, Garrett Casto wrote: > Is there any reason why we would ever want to have overlapping Accept() call > outstanding, or is this just trying to give possible flexibility to Sync? Hmmm. I was thinking of erring on the side of flexibility, but I can't think of a solid use case for having multiple Accepts. Relaxing an interface down the line is easier than constraining it, so how about I just allow one at a time for now? https://codereview.chromium.org/2383533003/diff/80001/blimp/net/helium/helium... blimp/net/helium/helium_transport.h:43: virtual void Accept(const StreamCreatedCallback& cb) = 0; On 2016/10/05 20:40:44, perumaal wrote: > There is no Accept in grpc land. Could we just have the Connect be > SetOnConnectionAccepted(...) callback and merge both? What Garrett said.
Switched to a delegate interface for the message receipt events.
lgtm https://codereview.chromium.org/2383533003/diff/100001/blimp/net/helium/heliu... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/100001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:49: void SetDelegate(std::unique_ptr<Delegate> delegate); nit: Delegates in general (IMO) are better off being WeakPtrs. Specifically, given that the caller of HeliumStream would need to somehow add 'itself' or an object it owns....
https://codereview.chromium.org/2383533003/diff/100001/blimp/net/helium/heliu... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/100001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:49: void SetDelegate(std::unique_ptr<Delegate> delegate); On 2016/10/06 00:02:11, perumaal wrote: > nit: Delegates in general (IMO) are better off being WeakPtrs. > Specifically, given that the caller of HeliumStream would need to somehow add > 'itself' or an object it owns.... Not sure if I agree. WeakPtrs are definitely good when destruction order is indeterminate. However, since the SyncManager is the both recipient of delegate events AND the owner of HeliumStreams, we can guarantee that the Stream will be torn down before the target Delegate.
On 2016/10/06 at 00:44:32, kmarshall wrote: > https://codereview.chromium.org/2383533003/diff/100001/blimp/net/helium/heliu... > File blimp/net/helium/helium_stream.h (right): > > https://codereview.chromium.org/2383533003/diff/100001/blimp/net/helium/heliu... > blimp/net/helium/helium_stream.h:49: void SetDelegate(std::unique_ptr<Delegate> delegate); > On 2016/10/06 00:02:11, perumaal wrote: > > nit: Delegates in general (IMO) are better off being WeakPtrs. > > Specifically, given that the caller of HeliumStream would need to somehow add > > 'itself' or an object it owns.... > > Not sure if I agree. WeakPtrs are definitely good when destruction order is indeterminate. However, since the SyncManager is the both recipient of delegate events AND the owner of HeliumStreams, we can guarantee that the Stream will be torn down before the target Delegate. Ok sg. Mainly wondering how the SyncManager is going to have a delegate that can talk to it while being lifetime-managed by the HeliumStream.
It could be a bare pointer, but one of the bits of advice I received in past code reviews was to pass delegates as unique pointers as a matter of practice as a way of guaranteeing delegate object lifetime to the owning class. The owner of the delegate doesn't need to understand anything about the lifetime of other objects - the delegate is *always* valid as far as the owner concerned. The nice thing is that the owner of the delegate is insulated from any kind of changes related to thread hopping or lifetime issues. If we make some changes and alter the ownership story, and find that we need to use a WeakPtr somewhere, that detail is completely encapsulated within the delegate object. Nothing changes from the delegate owner's point of view. Similarly, if we find that we need to jump threads from the delegate invocation site to the delegate target method, the owned Delegate proxy object would be a natural place to encapsulate the thread-hopping logic. Again, I don't think that this particular case is likely to change in that way, as the owner and the delegate producer are the same entity. A bare pointer could work. I just think this is just generally a nice clean habit to employ, esp. in an API's foundation layers.
On 2016/10/06 at 17:06:49, kmarshall wrote: > It could be a bare pointer, but one of the bits of advice I received in past code reviews was to pass delegates as unique pointers as a matter of practice as a way of guaranteeing delegate object lifetime to the owning class. The owner of the delegate doesn't need to understand anything about the lifetime of other objects - the delegate is *always* valid as far as the owner concerned. > > The nice thing is that the owner of the delegate is insulated from any kind of changes related to thread hopping or lifetime issues. If we make some changes and alter the ownership story, and find that we need to use a WeakPtr somewhere, that detail is completely encapsulated within the delegate object. Nothing changes from the delegate owner's point of view. Similarly, if we find that we need to jump threads from the delegate invocation site to the delegate target method, the owned Delegate proxy object would be a natural place to encapsulate the thread-hopping logic. > > Again, I don't think that this particular case is likely to change in that way, as the owner and the delegate producer are the same entity. A bare pointer could work. I just think this is just generally a nice clean habit to employ, esp. in an API's foundation layers. Thanks for patiently explaining. This is always tricky and I totally agree that the unique_ptr would be preferable to anything else as it has a clear sense of ownership.
lgtm https://codereview.chromium.org/2383533003/diff/100001/blimp/net/helium/heliu... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/100001/blimp/net/helium/heliu... blimp/net/helium/helium_transport.h:30: // Called when the transport transitions between an offline/unsuable state typo: unusable
lgtm
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lethalantidote@chromium.org, gcasto@chromium.org Link to the patchset: https://codereview.chromium.org/2383533003/#ps100001 (title: "ReadMessage delegate")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kmarshall@chromium.org
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scf@chromium.org, perumaal@chromium.org, gcasto@chromium.org, lethalantidote@chromium.org Link to the patchset: https://codereview.chromium.org/2383533003/#ps120001 (title: "typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:20: class HeliumStream { Can we break this into a interface and a separate base-class, plz? https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:29: // handler, not during. nit: Could we arrange things to make this safe? If we switched to a traditional ReceiveMessage() interface instead of a Delegate then we could signal disconnects by having that return an EOF value. The Delegate approach seems more complex by comparison. https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_transport.h:32: virtual void OnAvailabilityChanged(bool available) = 0; nit: Since this has only a single method right now, how about using a Callback and switching to Delegate if/when we add further Transport-level notifications? https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_transport.h:39: virtual void Connect(const StreamCreatedCallback& cb) = 0; As discussed previously, I think we need a way to cancel a pending Connect - otherwise if we try to Connect a new stream for an object, and receive a stream assigned to it before the Connect succeeds, we have to actually wait for the Connect to complete before we can close the redundant stream. https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_transport.h:42: // Only one Accept() call may be issued at a time. Do we need that restriction, really? We could make this symmetric with Connect simply by maintaining a queue of StreamCreatedCallback calls to satisfy with received connections. :) https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_transport.h:47: virtual bool IsAvailable() = 0; nit: Do we need both a notification *and* this?
https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:20: class HeliumStream { On 2016/10/08 01:02:29, Wez wrote: > Can we break this into a interface and a separate base-class, plz? Done. https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:29: // handler, not during. On 2016/10/08 01:02:29, Wez wrote: > nit: Could we arrange things to make this safe? > > If we switched to a traditional ReceiveMessage() interface instead of a Delegate > then we could signal disconnects by having that return an EOF value. The > Delegate approach seems more complex by comparison. I've been getting a lot of mixed signals from a lot of parties about using a delegate vs. a ReceiveMessage callback. Matt and Garrett are in favor of the delegate approach. I'm rather on the fence. When Matt pointed out that we should slurp data from the pipe as fast as possible as a matter of policy so that we don't create any pushback signals, I thought that my big argument for going with a callback was rendered moot. My inclination is to leave it as-is since IMO the matter of additional complexity is relative/debatable... https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... File blimp/net/helium/helium_transport.h (right): https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_transport.h:32: virtual void OnAvailabilityChanged(bool available) = 0; On 2016/10/08 01:02:29, Wez wrote: > nit: Since this has only a single method right now, how about using a Callback > and switching to Delegate if/when we add further Transport-level notifications? Done. https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_transport.h:39: virtual void Connect(const StreamCreatedCallback& cb) = 0; On 2016/10/08 01:02:29, Wez wrote: > As discussed previously, I think we need a way to cancel a pending Connect - > otherwise if we try to Connect a new stream for an object, and receive a stream > assigned to it before the Connect succeeds, we have to actually wait for the > Connect to complete before we can close the redundant stream. I switched to CancelableCallback. The Stream that is std::move'd to the callback will be destroyed when the CancelableCallback no-ops Run(). As an optimization, the Transport can GC cancelled callbacks prior to invocation, so the Stream is given to a worthier live callback. https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_transport.h:42: // Only one Accept() call may be issued at a time. On 2016/10/08 01:02:30, Wez wrote: > Do we need that restriction, really? We could make this symmetric with Connect > simply by maintaining a queue of StreamCreatedCallback calls to satisfy with > received connections. :) Done. https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_transport.h:47: virtual bool IsAvailable() = 0; On 2016/10/08 01:02:29, Wez wrote: > nit: Do we need both a notification *and* this? I think it's useful for querying the initial connected state from the Transport. The notification as it is specified now is only fired for state transitions, which IMO makes the execution flow more straightforward for the notification recipient.
https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/120001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:29: // handler, not during. On 2016/10/11 20:02:37, Kevin M wrote: > On 2016/10/08 01:02:29, Wez wrote: > > nit: Could we arrange things to make this safe? > > > > If we switched to a traditional ReceiveMessage() interface instead of a > Delegate > > then we could signal disconnects by having that return an EOF value. The > > Delegate approach seems more complex by comparison. > > I've been getting a lot of mixed signals from a lot of parties about using a > delegate vs. a ReceiveMessage callback. Matt and Garrett are in favor of the > delegate approach. I'm rather on the fence. When Matt pointed out that we should > slurp data from the pipe as fast as possible as a matter of policy so that we > don't create any pushback signals, I thought that my big argument for going with > a callback was rendered moot. > > My inclination is to leave it as-is since IMO the matter of additional > complexity is relative/debatable... To clarify my stance, I was originally for the delegate but after talking with both Kevin and Matt, I basically think that it's a toss up. Flow control doesn't seem like a motivating factor and the overall complexity seems about the same in both cases. Somewhere there is a loop that is going to buffer data since we don't want to apply multiple changesets at the same time (assuming that in some cases changeset application is async). At this point it feels like we should implement something and we can figure refine the interface as necessary. As is, it seems like there are two issues though: - OnMessageReceived() doesn't identify the stream in any way, which seems like something that the Sync layer would want. The original design for this used a callback so that the Sync layer could choose whatever identifier it wanted for each Stream. - I think that you would want to specify a HeliumResult in OnStreamDisconnected() (or as another parameter to OnMessageRecieved()) since there may be different reasons for disconnection and the caller may react to them differently.
I think it makes a lot of sense to place the pump outside the transport implementation, as it seems like common behavior that we'll want regardless of the transport type. I'll switch this back to ReceiveMessage(). As you mentioned, the Callback interface give callers the freedom to partially bind arbitrary information, like the stream ID and so on. Added a HeliumResult param to OnStreamDisconnected.
On 2016/10/11 at 21:53:39, kmarshall wrote: > I think it makes a lot of sense to place the pump outside the transport implementation, as it seems like common behavior that we'll want regardless of the transport type. > > I'll switch this back to ReceiveMessage(). As you mentioned, the Callback interface give callers the freedom to partially bind arbitrary information, like the stream ID and so on. > > Added a HeliumResult param to OnStreamDisconnected. Would ReceiveMessage receive one message at a time? (i.e. poll?)
Only one message. I don't think this is polling because there is at most only one ReceiveMessage() call per message received, though. IIRC, polling consists of regular checks of "got any messages for me? how about now? and now? and now?" Here, if a message isn't immediately available, the callback is retained for asynchronous invocation once a message arrives.
On 2016/10/11 at 22:22:27, kmarshall wrote: > Only one message. I don't think this is polling because there is at most only one ReceiveMessage() call per message received, though. IIRC, polling consists of regular checks of "got any messages for me? how about now? and now? and now?" > > Here, if a message isn't immediately available, the callback is retained for asynchronous invocation once a message arrives. Got it, SGTM. thanks!
On last thing. Otherwise LGTM. https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:31: HeliumResult reason) = 0; We shouldn't need this and ReceiveMessage(). If we are disconnected, ReceiveMessage() should just invoke the callback with HeliumResult::DISCONNECTED. https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:52: // incoming messages to ensure proper pushback w/transport-level flow control. Nit: Sounds like we explicitly are okay with pre-emptively buffering at this point, so we can probably remove this sentence.
https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:54: base::Callback<void(std::unique_ptr<HeliumMessage>, HeliumResult)> Could we alias this as HeliumMsgReceivedCallback?
lgtm
https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:46: const base::Callback<void(HeliumResult)>& callback) = 0; Ah I now see this problem: The caller of SendMessage would need to bookkeep the list of messages... (As they cannot send a message until the callback is invoked). This seems unnecessary complexity to me and would result in some sort of locks *above* the stream layer (specifically the sync layer). Can we just remove the 'The caller is responsible...'? Based on my current implementation, this detail (of buffering during sending) lives in the stream implementation not in the caller. Note that for *receiving* this is a completely different story: you can basically buffer as you want (or *not*, which is also fine) and this imposes no locks or book-keeping in any layer including the stream (as the completion queue would take care of it for us).
https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... File blimp/net/helium/helium_stream.h (right): https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:31: HeliumResult reason) = 0; On 2016/10/11 22:33:22, Garrett Casto wrote: > We shouldn't need this and ReceiveMessage(). If we are disconnected, > ReceiveMessage() should just invoke the callback with > HeliumResult::DISCONNECTED. Done. https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:46: const base::Callback<void(HeliumResult)>& callback) = 0; On 2016/10/11 23:09:39, perumaal wrote: > Ah I now see this problem: The caller of SendMessage would need to bookkeep the > list of messages... > (As they cannot send a message until the callback is invoked). This seems > unnecessary complexity to me and would result in some sort of locks *above* the > stream layer (specifically the sync layer). That's by design, actually. The construction of HeliumMessages is done lazily, at the time of delivery to the transport layer. This means that when we are clear to send, the data that we transmit is as fresh as we can make it, w/o buffered clutter from bursts of modifications. https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:52: // incoming messages to ensure proper pushback w/transport-level flow control. On 2016/10/11 22:33:22, Garrett Casto wrote: > Nit: Sounds like we explicitly are okay with pre-emptively buffering at this > point, so we can probably remove this sentence. Done. https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... blimp/net/helium/helium_stream.h:54: base::Callback<void(std::unique_ptr<HeliumMessage>, HeliumResult)> On 2016/10/11 22:36:36, perumaal wrote: > Could we alias this as HeliumMsgReceivedCallback? Done.
Removed helium_stream.cc from the CL, now that everything is pure virtual.
The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM.
> > https://codereview.chromium.org/2383533003/diff/160001/blimp/net/helium/heliu... > blimp/net/helium/helium_stream.h:46: const base::Callback<void(HeliumResult)>& callback) = 0; > On 2016/10/11 23:09:39, perumaal wrote: > > Ah I now see this problem: The caller of SendMessage would need to bookkeep the > > list of messages... > > (As they cannot send a message until the callback is invoked). This seems > > unnecessary complexity to me and would result in some sort of locks *above* the > > stream layer (specifically the sync layer). > > That's by design, actually. The construction of HeliumMessages is done lazily, at the time of delivery to the transport layer. This means that when we are clear to send, the data that we transmit is as fresh as we can make it, w/o buffered clutter from bursts of modifications. Okay. From the transport layer perspective, there is no actual throttling. So I guess if you'd like to throttle above that, it should be fine (but not sure if that should be prescriptive/constrained from the API itself).... For instance, BlimpConnection does no throttling so for now, I am just routing messages at the speed they pass through BlimpConnection and the callbacks are all 'FYI'. This could be revisited later.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scf@chromium.org, lethalantidote@chromium.org, perumaal@chromium.org Link to the patchset: https://codereview.chromium.org/2383533003/#ps180001 (title: "code review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Blimp: define HeliumTransport/HeliumStream interfaces. These classes encapsulate transport-specific code for connecting logical bidirectional message streams, and the logic for sending and receiving messages on those streams. R=scf@chromium.org,perumaal@chromium.org,gcasto@chromium.org CC=lethalantidote@chromium.org,steimel@chromium.org BUG=651504 ========== to ========== Blimp: define HeliumTransport/HeliumStream interfaces. These classes encapsulate transport-specific code for connecting logical bidirectional message streams, and the logic for sending and receiving messages on those streams. R=scf@chromium.org,perumaal@chromium.org,gcasto@chromium.org CC=lethalantidote@chromium.org,steimel@chromium.org BUG=651504 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Blimp: define HeliumTransport/HeliumStream interfaces. These classes encapsulate transport-specific code for connecting logical bidirectional message streams, and the logic for sending and receiving messages on those streams. R=scf@chromium.org,perumaal@chromium.org,gcasto@chromium.org CC=lethalantidote@chromium.org,steimel@chromium.org BUG=651504 ========== to ========== Blimp: define HeliumTransport/HeliumStream interfaces. These classes encapsulate transport-specific code for connecting logical bidirectional message streams, and the logic for sending and receiving messages on those streams. R=scf@chromium.org,perumaal@chromium.org,gcasto@chromium.org CC=lethalantidote@chromium.org,steimel@chromium.org BUG=651504 Committed: https://crrev.com/e97d5665eb5f7e7680798a8a0c59fdb8d10817a7 Cr-Commit-Position: refs/heads/master@{#424870} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e97d5665eb5f7e7680798a8a0c59fdb8d10817a7 Cr-Commit-Position: refs/heads/master@{#424870} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
