Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
79673003:
Refactor CastSocket code for the following: (Closed)
|
Created:
7 years, 1 month ago by Munjal (Google) Modified:
6 years, 11 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionRefactor CastSocket code for the following:
- Refactor the Write code to avoid recursion
- Refactor the read code to avoid recursion
- With read code refactored, hook it up appropriately with connection flow
to avoid re-entrancy of DoConnectLoop
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242158
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 11
Patch Set 7 : #Patch Set 8 : #
Total comments: 36
Patch Set 9 : #Patch Set 10 : #
Total comments: 11
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #
Total comments: 5
Patch Set 16 : #Patch Set 17 : #
Total comments: 34
Patch Set 18 : #
Total comments: 10
Patch Set 19 : #Patch Set 20 : #Patch Set 21 : #Patch Set 22 : #Patch Set 23 : #Patch Set 24 : #Patch Set 25 : #Patch Set 26 : #
Total comments: 23
Patch Set 27 : #Patch Set 28 : #
Total comments: 6
Patch Set 29 : #Patch Set 30 : #
Total comments: 2
Patch Set 31 : #Messages
Total messages: 43 (0 generated)
Note that this is just for the initial review of hte approach. I am going to work on unit tests next (they won't even compile probably at this point). But I wanted to get an overall opinion on the approach first.
https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:157: void CastSocket::Connect(const net::CompletionCallback& callback) { This is an area where it seems like you could further adopt the net/ pattern of sync-or-async return (much as we do for Write and Read) Namely int CastSocket::Connect(const net::CompletionCallback& callback) { int rv = ConnectInternal(); if (rv != net::ERR_IO_PENDING) return rv; connect_callback_ = callback; return net::ERR_IO_PENDING; } Then the caller can handle synchronous completion. This avoids a mandatory task hop, which can introduce performance issues on mobile (where, admittedly, this is less critical) https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:333: // Seems safe to invoke the callback synchronously for validation errors. This comment doesn't really help readers of this code; If you're not sure about it, then it's unsafe, and if you are sure about it, say so :) It creates confusion as to what the safe API guarantees are - and this is your API, you get to make the rules. https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:345: PostTaskToSendCastMessage(message_proto, callback); I don't understand why you need to Post a task to do this. Properly designed, you shouldn't need to do a context switch here - you should be able to synchronously read (and possibly fail) and invoke the callback safely. There are other ways to do this that also avoid the PostTask overhead (both mental and computational), IF it's absolutely necessary (and reading this code in isolation doesn't seem to suggest it is), such as using a RefCounted or WeakPtr'd inner class that holds onto whatever data members you need after the callback. The way net/ sockets are used, it's the higher layers responsibility to only write one message at a time; that is, if there is an outstanding write that returned ERR_IO_PENDING, the caller should not Write more data until the Write completion callback is returned. This moves the queuing logic up a layer. I realize you've adopted that queuing logic here, but I'm wondering whether it makes sense to push it up even one layer further (I'm not sure what that layer looks like, mind you), to handle the message queue. Just $.02 https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:429: request.callback.Run(result); If the callback deletes this object, transitively, then you'll have a memory error on line 430. Seems like you need to shuffle some of this error handling (and make sure DoWriteCompelte doesn't access member variables in its unwind path) https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:446: write_state_ = WRITE_STATE_WRITE; BUG: You don't handle == 0 (aka EOF).
https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:157: void CastSocket::Connect(const net::CompletionCallback& callback) { On 2013/11/21 22:16:15, Ryan Sleevi wrote: > This is an area where it seems like you could further adopt the net/ pattern of > sync-or-async return (much as we do for Write and Read) > > Namely > > int CastSocket::Connect(const net::CompletionCallback& callback) { > int rv = ConnectInternal(); > if (rv != net::ERR_IO_PENDING) > return rv; > connect_callback_ = callback; > return net::ERR_IO_PENDING; > } > > Then the caller can handle synchronous completion. This avoids a mandatory task > hop, which can introduce performance issues on mobile (where, admittedly, this > is less critical) I was trying to avoid invoking the callback synchronously from Connect. https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:345: PostTaskToSendCastMessage(message_proto, callback); On 2013/11/21 22:16:15, Ryan Sleevi wrote: > I don't understand why you need to Post a task to do this. Properly designed, > you shouldn't need to do a context switch here - you should be able to > synchronously read (and possibly fail) and invoke the callback safely. > > There are other ways to do this that also avoid the PostTask overhead (both > mental and computational), IF it's absolutely necessary (and reading this code > in isolation doesn't seem to suggest it is), such as using a RefCounted or > WeakPtr'd inner class that holds onto whatever data members you need after the > callback. > > The way net/ sockets are used, it's the higher layers responsibility to only > write one message at a time; that is, if there is an outstanding write that > returned ERR_IO_PENDING, the caller should not Write more data until the Write > completion callback is returned. This moves the queuing logic up a layer. > > I realize you've adopted that queuing logic here, but I'm wondering whether it > makes sense to push it up even one layer further (I'm not sure what that layer > looks like, mind you), to handle the message queue. Just $.02 Actually, we do not need to post a task here. When SendMessage is called, it is safe to invoke the callback synchronously since the caller should not delete this object in the write callback. If a write loop is already running we will return asynchronously anyway. I called PostTask* here by mistake. Changed now. Pushing the queuing logic outside does not make sense since the consumer of this class is the extension API implementation and this class is supposed to be the abstraction. We could introduce another class in the middle, but that would just move the queuing logic from here to there. https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:429: request.callback.Run(result); On 2013/11/21 22:16:15, Ryan Sleevi wrote: > If the callback deletes this object, transitively, then you'll have a memory > error on line 430. > > Seems like you need to shuffle some of this error handling (and make sure > DoWriteCompelte doesn't access member variables in its unwind path) The caller should not delete this object in the write callback. It should delete this object in the OnError callback of the delegate. https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:446: write_state_ = WRITE_STATE_WRITE; On 2013/11/21 22:16:15, Ryan Sleevi wrote: > BUG: You don't handle == 0 (aka EOF). I looked at the code in spdy_session.cc and (wrongly?) assumed that 0 is never returned by write. I will find out and add the appropriate handling here.
https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:157: void CastSocket::Connect(const net::CompletionCallback& callback) { On 2013/11/21 22:44:11, Munjal (Google) wrote: > On 2013/11/21 22:16:15, Ryan Sleevi wrote: > > This is an area where it seems like you could further adopt the net/ pattern > of > > sync-or-async return (much as we do for Write and Read) > > > > Namely > > > > int CastSocket::Connect(const net::CompletionCallback& callback) { > > int rv = ConnectInternal(); > > if (rv != net::ERR_IO_PENDING) > > return rv; > > connect_callback_ = callback; > > return net::ERR_IO_PENDING; > > } > > > > Then the caller can handle synchronous completion. This avoids a mandatory > task > > hop, which can introduce performance issues on mobile (where, admittedly, this > > is less critical) > > I was trying to avoid invoking the callback synchronously from Connect. Actually I did not explain my reasons very well. It is fine to invoke the callback synchronously as long as I make sure I don't access member veriables in the unwind path (like you said) since the caller may delete this object in the callback. BUT Wez suggested that may be it is cleaner to have DoConnectLoop as the callback for various async operations (instead of OnConnectComplete, OnChallengeEvent, etc.). I also observed that in the previous version of the code there were a few places where I was doing: int rv = DoConnectLoop(result); if (rv != net::ERR_IO_PENDING) DoConnectCallback(rv); So I agreed with Wez's suggestion. So I thought if DoConnectLoop also takes car of calling the connect callback as part of its flow, then I can address the above points. In doing that, I needed to make DoConnectLoop return type void instead of int. And that seemed OK. I can make DoConnectLoop return int again and then invoke the callback synchronously if needed, but that will introduce another method, say OnConnectCallback. Then there will be two places - Connect and OnConnectCallback - which need to do the code snippet above.
https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/230001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:157: void CastSocket::Connect(const net::CompletionCallback& callback) { On 2013/11/21 22:53:36, Munjal (Google) wrote: > On 2013/11/21 22:44:11, Munjal (Google) wrote: > > On 2013/11/21 22:16:15, Ryan Sleevi wrote: > > > This is an area where it seems like you could further adopt the net/ pattern > > of > > > sync-or-async return (much as we do for Write and Read) > > > > > > Namely > > > > > > int CastSocket::Connect(const net::CompletionCallback& callback) { > > > int rv = ConnectInternal(); > > > if (rv != net::ERR_IO_PENDING) > > > return rv; > > > connect_callback_ = callback; > > > return net::ERR_IO_PENDING; > > > } > > > > > > Then the caller can handle synchronous completion. This avoids a mandatory > > task > > > hop, which can introduce performance issues on mobile (where, admittedly, > this > > > is less critical) > > > > I was trying to avoid invoking the callback synchronously from Connect. > > Actually I did not explain my reasons very well. It is fine to invoke the > callback synchronously as long as I make sure I don't access member veriables in > the unwind path (like you said) since the caller may delete this object in the > callback. > > BUT Wez suggested that may be it is cleaner to have DoConnectLoop as the > callback for various async operations (instead of OnConnectComplete, > OnChallengeEvent, etc.). I also observed that in the previous version of the > code there were a few places where I was doing: > > int rv = DoConnectLoop(result); > if (rv != net::ERR_IO_PENDING) > DoConnectCallback(rv); > > So I agreed with Wez's suggestion. > > So I thought if DoConnectLoop also takes car of calling the connect callback as > part of its flow, then I can address the above points. In doing that, I needed > to make DoConnectLoop return type void instead of int. And that seemed OK. > > I can make DoConnectLoop return int again and then invoke the callback > synchronously if needed, but that will introduce another method, say > OnConnectCallback. Then there will be two places - Connect and OnConnectCallback > - which need to do the code snippet above. Here is an approach that might work better given all of the above: - Add a method, say void PumpWriteLoop, that all places call - Make PumpWriteLoop the callback method for all async operations DoConnectLoop calls - Remove the PostTask from Connect What do you think?
Can you re-upload the patch? Latest one seems to be broken on Rietveld.
Wez, I uploaded the patch again. Let me know if it works now (I checked it and seems like SxS diff are showing up good now).
https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:160: // Post a task so that we can safely run the callback You mean so that if the Connect() completes synchronously then the callback can be invoked from within the Connect impl? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:169: if (ready_state_ != READY_STATE_NONE) { Under what circumstances could |ready_state_| not be NONE when we reach here? Should this be a DCHECK? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:292: if (result < 0) Didn't we already fix these? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:352: message, callback)); This means you're posting a task on every send; why not try the send, and post the completion callback if it completes synchronously? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:413: write_state_ = WRITE_STATE_WRITE_COMPLETE; Surely at this point the write is pending? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:428: request.callback.Run(result); If the write was triggered by connect-time activity then surely you can end up in DoConnectLoop, called from DoWriteComplete, via DoWriteLoop. Is that safe? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:445: write_state_ = WRITE_STATE_WRITE; Under what circumstances do we ever return to WRITE_STATE_NONE? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:463: base::Bind(&CastSocket::StartReadLoop, AsWeakPtr())); As for read, why do this? Why not start the read directly, and post a task only if it completes synchronously, to handle the result? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:188: void ConnectInternal(); So is this not part of DoConnectLoop, then? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:193: bool ParseChannelUrl(const GURL& url); nit: Why is this mixed-in in between the Connect and write impl methods? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:208: // Posts a task to send the given message. Why are these methods mixed-in between the write & read impls?
https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:160: // Post a task so that we can safely run the callback On 2013/11/26 02:45:56, Wez wrote: > You mean so that if the Connect() completes synchronously then the callback can > be invoked from within the Connect impl? Actually I removed this PostTask. Upon thinking about it more, it is safe to invoke callback synchronously in error cases or in cases when net operations finish synchronously as long as we don't access data members in the callstack unwind path since the caller can delete this in the callback. This was per suggestion from Ryan. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:169: if (ready_state_ != READY_STATE_NONE) { On 2013/11/26 02:45:56, Wez wrote: > Under what circumstances could |ready_state_| not be NONE when we reach here? > Should this be a DCHECK? When the caller calls Connect twice. In that case we invoke the callback with error. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:292: if (result < 0) On 2013/11/26 02:45:56, Wez wrote: > Didn't we already fix these? Yes, we did but I refactored that method a bit and now I consistently handle the result appropriate in Do* method. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:352: message, callback)); On 2013/11/26 02:45:56, Wez wrote: > This means you're posting a task on every send; why not try the send, and post > the completion callback if it completes synchronously? We don't post a task for every SendMessage. We only post a task if we are sending the auth challenge. SendMessage method directly calls SendMessageInternal. But SendAuthChallenge calls PostTaskToSendMessage. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:413: write_state_ = WRITE_STATE_WRITE_COMPLETE; On 2013/11/26 02:45:56, Wez wrote: > Surely at this point the write is pending? Yes. But the caller - DoWriteLoop - will get an IO_PENDING as return value if the Write below does not finish sync'ly and in that case DoWriteLoop will wait to get a callback before doing DoWriteComplete https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:428: request.callback.Run(result); On 2013/11/26 02:45:56, Wez wrote: > If the write was triggered by connect-time activity then surely you can end up > in DoConnectLoop, called from DoWriteComplete, via DoWriteLoop. Is that safe? If the Write was triggered by connect-time activity, DOConnectLoop will not be on the stack since we always post a task to start DoWriteLoop. So we will be fine. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:445: write_state_ = WRITE_STATE_WRITE; On 2013/11/26 02:45:56, Wez wrote: > Under what circumstances do we ever return to WRITE_STATE_NONE? We never return to WRITE_STATE_NONE. We only start out in that state and we never return to that state. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:463: base::Bind(&CastSocket::StartReadLoop, AsWeakPtr())); On 2013/11/26 02:45:56, Wez wrote: > As for read, why do this? Why not start the read directly, and post a task only > if it completes synchronously, to handle the result? This felt cleaner and more robust since otherwise we have to keep state to figure out if read completed synchronously or not deep down into the helper methods so that it can do the right thing. And we have to this in every code path that does the callback (right now there is only one though). Let me know what you think. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:188: void ConnectInternal(); On 2013/11/26 02:45:56, Wez wrote: > So is this not part of DoConnectLoop, then? No. But now I removed it anyway. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:193: bool ParseChannelUrl(const GURL& url); On 2013/11/26 02:45:56, Wez wrote: > nit: Why is this mixed-in in between the Connect and write impl methods? This is a helper used by connection methods. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:208: // Posts a task to send the given message. On 2013/11/26 02:45:56, Wez wrote: > Why are these methods mixed-in between the write & read impls? These are all helper methods using during write flow.
https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:160: // Post a task so that we can safely run the callback On 2013/11/26 20:06:14, Munjal (Google) wrote: > On 2013/11/26 02:45:56, Wez wrote: > > You mean so that if the Connect() completes synchronously then the callback > can > > be invoked from within the Connect impl? > > Actually I removed this PostTask. Upon thinking about it more, it is safe to > invoke callback synchronously in error cases or in cases when net operations > finish synchronously as long as we don't access data members in the callstack > unwind path since the caller can delete this in the callback. This was per > suggestion from Ryan. That makes the code super-fragile to code changes that don't take the restriction into account, though; even with this CL as-is DoConnectLoop accesses connect_state_, a member of the object, after running a connection state function that may have triggered the callback - if the callback were to delete the CastSocket then that would lead to a use-after-free. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:169: if (ready_state_ != READY_STATE_NONE) { On 2013/11/26 20:06:14, Munjal (Google) wrote: > On 2013/11/26 02:45:56, Wez wrote: > > Under what circumstances could |ready_state_| not be NONE when we reach here? > > Should this be a DCHECK? > > When the caller calls Connect twice. In that case we invoke the callback with > error. You mean that JS calls a connect method twice? Or some other Chrome code? If the latter then DCHECK seems more appropriate. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:352: message, callback)); On 2013/11/26 20:06:14, Munjal (Google) wrote: > On 2013/11/26 02:45:56, Wez wrote: > > This means you're posting a task on every send; why not try the send, and post > > the completion callback if it completes synchronously? > > We don't post a task for every SendMessage. We only post a task if we are > sending the auth challenge. SendMessage method directly calls > SendMessageInternal. But SendAuthChallenge calls PostTaskToSendMessage. In that case can't SendAuthChallenge do the post directly? Why does it need bouncing through another helper method? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:413: write_state_ = WRITE_STATE_WRITE_COMPLETE; On 2013/11/26 20:06:14, Munjal (Google) wrote: > On 2013/11/26 02:45:56, Wez wrote: > > Surely at this point the write is pending? > > Yes. But the caller - DoWriteLoop - will get an IO_PENDING as return value if > the Write below does not finish sync'ly and in that case DoWriteLoop will wait > to get a callback before doing DoWriteComplete Right - so the write has _not_ completed - we're in a write-pending state; the write hasn't completed as far as we're concerned until the completion callback returns, at which point we return to an about-to-write or nothing-to-write state? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:428: request.callback.Run(result); On 2013/11/26 20:06:14, Munjal (Google) wrote: > On 2013/11/26 02:45:56, Wez wrote: > > If the write was triggered by connect-time activity then surely you can end up > > in DoConnectLoop, called from DoWriteComplete, via DoWriteLoop. Is that safe? > > If the Write was triggered by connect-time activity, DOConnectLoop will not be > on the stack since we always post a task to start DoWriteLoop. So we will be > fine. The case I'm asking about is DoWriteLoop -> DoWriteComplete -> DoConnectLoop. Are you sure that that is a safe nesting, including the case in which the caller deletes the socket in response to e.g. failed connect? https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:463: base::Bind(&CastSocket::StartReadLoop, AsWeakPtr())); On 2013/11/26 20:06:14, Munjal (Google) wrote: > On 2013/11/26 02:45:56, Wez wrote: > > As for read, why do this? Why not start the read directly, and post a task > only > > if it completes synchronously, to handle the result? > > This felt cleaner and more robust since otherwise we have to keep state to > figure out if read completed synchronously or not deep down into the helper > methods so that it can do the right thing. And we have to this in every code > path that does the callback (right now there is only one though). > > Let me know what you think. The amount of logic should be the same; what I'm suggesting is: <do work> Read(&Callback) <if synchronous, Post(Callback), else return> This way Callback is always handled asynchronously, and you avoid the Post() in the case where Read() completes asynchronously. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:193: bool ParseChannelUrl(const GURL& url); On 2013/11/26 20:06:14, Munjal (Google) wrote: > On 2013/11/26 02:45:56, Wez wrote: > > nit: Why is this mixed-in in between the Connect and write impl methods? > > This is a helper used by connection methods. OK; I think it would help to clarify that, e.g. "Called by <connect method> to verify |url| and, if valid, assign it to |url_|. Returns true if |url| is a valid cast[s]:// URL." https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:208: // Posts a task to send the given message. On 2013/11/26 20:06:14, Munjal (Google) wrote: > On 2013/11/26 02:45:56, Wez wrote: > > Why are these methods mixed-in between the write & read impls? > > These are all helper methods using during write flow. OK; as above, I think it'd help to have a block comment introducing them as such. (But that's a nit ;)
Adding mfoltz as reviewer https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:160: // Post a task so that we can safely run the callback On 2013/11/27 04:15:19, Wez wrote: > On 2013/11/26 20:06:14, Munjal (Google) wrote: > > On 2013/11/26 02:45:56, Wez wrote: > > > You mean so that if the Connect() completes synchronously then the callback > > can > > > be invoked from within the Connect impl? > > > > Actually I removed this PostTask. Upon thinking about it more, it is safe to > > invoke callback synchronously in error cases or in cases when net operations > > finish synchronously as long as we don't access data members in the callstack > > unwind path since the caller can delete this in the callback. This was per > > suggestion from Ryan. > > That makes the code super-fragile to code changes that don't take the > restriction into account, though; even with this CL as-is DoConnectLoop accesses > connect_state_, a member of the object, after running a connection state > function that may have triggered the callback - if the callback were to delete > the CastSocket then that would lead to a use-after-free. Several things to keep in mind: - DoConnectLoop does not access connect_state_ after invoking the connect callback. Connection callback is only invoked in one place in the file - inside DoConnectCallback - which is called in one cplace only - outside the do/while loop in DoConnectLoop. - Whether or not we post a task, we have to ensure that it is safe for the caller to delete the socket in the connection callback https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:169: if (ready_state_ != READY_STATE_NONE) { On 2013/11/27 04:15:19, Wez wrote: > On 2013/11/26 20:06:14, Munjal (Google) wrote: > > On 2013/11/26 02:45:56, Wez wrote: > > > Under what circumstances could |ready_state_| not be NONE when we reach > here? > > > Should this be a DCHECK? > > > > When the caller calls Connect twice. In that case we invoke the callback with > > error. > > You mean that JS calls a connect method twice? Or some other Chrome code? If the > latter then DCHECK seems more appropriate. Yes, if Js calls connect method twice. Of course DCHECK would be fine if it cannot be done external to Chrome code. But error seems like a better way to ahndle it since it can be triggered externally. Another way would be just to ignore all calls to Connect except the first one. Let me know if you prefer that. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:352: message, callback)); On 2013/11/27 04:15:19, Wez wrote: > On 2013/11/26 20:06:14, Munjal (Google) wrote: > > On 2013/11/26 02:45:56, Wez wrote: > > > This means you're posting a task on every send; why not try the send, and > post > > > the completion callback if it completes synchronously? > > > > We don't post a task for every SendMessage. We only post a task if we are > > sending the auth challenge. SendMessage method directly calls > > SendMessageInternal. But SendAuthChallenge calls PostTaskToSendMessage. > > In that case can't SendAuthChallenge do the post directly? Why does it need > bouncing through another helper method? Done. I thought it might be handy to have this helper in case we need it from multiple places. But if we do that, we can add it later. Removed now. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:413: write_state_ = WRITE_STATE_WRITE_COMPLETE; On 2013/11/27 04:15:19, Wez wrote: > On 2013/11/26 20:06:14, Munjal (Google) wrote: > > On 2013/11/26 02:45:56, Wez wrote: > > > Surely at this point the write is pending? > > > > Yes. But the caller - DoWriteLoop - will get an IO_PENDING as return value if > > the Write below does not finish sync'ly and in that case DoWriteLoop will wait > > to get a callback before doing DoWriteComplete > > Right - so the write has _not_ completed - we're in a write-pending state; the > write hasn't completed as far as we're concerned until the completion callback > returns, at which point we return to an about-to-write or nothing-to-write > state? I think the confusion is in the name - this is the "next" write state to be executed but ONLY WHEN the current operation completes. This is how the code in net does things - it sets the state to the next state but returns IO_PENDING to indicate that that cannot be executed yet. I think it keeps the number of states manageable. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:428: request.callback.Run(result); On 2013/11/27 04:15:19, Wez wrote: > On 2013/11/26 20:06:14, Munjal (Google) wrote: > > On 2013/11/26 02:45:56, Wez wrote: > > > If the write was triggered by connect-time activity then surely you can end > up > > > in DoConnectLoop, called from DoWriteComplete, via DoWriteLoop. Is that > safe? > > > > If the Write was triggered by connect-time activity, DOConnectLoop will not be > > on the stack since we always post a task to start DoWriteLoop. So we will be > > fine. > > The case I'm asking about is DoWriteLoop -> DoWriteComplete -> DoConnectLoop. > Are you sure that that is a safe nesting, including the case in which the caller > deletes the socket in response to e.g. failed connect? You are right. Fixed now. We should not access any member variables after invoking the callback. https://codereview.chromium.org/79673003/diff/330001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:463: base::Bind(&CastSocket::StartReadLoop, AsWeakPtr())); On 2013/11/27 04:15:19, Wez wrote: > On 2013/11/26 20:06:14, Munjal (Google) wrote: > > On 2013/11/26 02:45:56, Wez wrote: > > > As for read, why do this? Why not start the read directly, and post a task > > only > > > if it completes synchronously, to handle the result? > > > > This felt cleaner and more robust since otherwise we have to keep state to > > figure out if read completed synchronously or not deep down into the helper > > methods so that it can do the right thing. And we have to this in every code > > path that does the callback (right now there is only one though). > > > > Let me know what you think. > > The amount of logic should be the same; what I'm suggesting is: > > <do work> > Read(&Callback) > <if synchronous, Post(Callback), else return> > > This way Callback is always handled asynchronously, and you avoid the Post() in > the case where Read() completes asynchronously. If it finishes synchronously then it will invoke the callback already deep inside hte stack. So the way to detect that would be to add a boolean - in_read_loop_ - which is set to true here and then checked in ParseMessageFromBody. May be the right thin to do is to refactor the call to DoConnectLoop outside of ParseMessageBody and add one more state for that?
Looks good as far as factoring out the state machines for read and write. Mostly minor requests for comments, and wanted to discuss the need for PostTask-ing to the same thread - wondering if this was the cleanest way to avoid re-entrancy. https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:275: // Post a task to send a challenge message to avoid re-entrancy Not a big fan of posting a task to implement a control flow in the same thread. Would it be possible to introduce a new state CONN_STATE_AUTH_CHALLENGE_SEND_PENDING or some such and re-enter the DoConnectLoop to avoid this? https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:370: int rv = result; You might add a brief comment referring the reader to DoConnectLoop() to explain the control flow style here, or add a different version to describe the interaction with write_queue_. https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:421: if (result > 0) { Thank you, this is clearer and easier to read. https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:468: Add a brief comment describing the control flow and loop invariant. https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:590: DoConnectLoop(net::OK); So the flow here is DoConnectLoop -> DoAuthChallengeSendComplete -> post task to StartReadLoop -> DoReadLoop -> DoReadComplete -> ProcessBody -> ParseMessageFromBody -> DoConnectLoop (having advanced the state to CONN_STATE_AUTH_CHALLENGE_REPLY_COMPLETE). To eliminate the PostTask, we could terminate the first invocation of the DoReadLoop() terminate on receiving the auth reply and return up to DoConnectLoop(), and later restart the read loop after the connection completes. I am not sure if that improves the code or not though. https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:227: // Stars the read loop if not already started. s/Stars/Starts/
https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:275: // Post a task to send a challenge message to avoid re-entrancy On 2013/12/03 00:56:00, mark a. foltz wrote: > Not a big fan of posting a task to implement a control flow in the same thread. > Would it be possible to introduce a new state > CONN_STATE_AUTH_CHALLENGE_SEND_PENDING or some such and re-enter the > DoConnectLoop to avoid this? Even if we do what you are suggesting - return back to DoConnectLoop and then start a read loop - we will still not avoid the re-entrancy issue since in that case we will still have DoConnectLoop > DoReadLoop > ... > DoConnectLoop. But I am thinking of another approach to avoid posting a task. Let me try that out. https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:370: int rv = result; On 2013/12/03 00:56:00, mark a. foltz wrote: > You might add a brief comment referring the reader to DoConnectLoop() to explain > the control flow style here, or add a different version to describe the > interaction with write_queue_. Done. https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:421: if (result > 0) { On 2013/12/03 00:56:00, mark a. foltz wrote: > Thank you, this is clearer and easier to read. Sure. https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:468: On 2013/12/03 00:56:00, mark a. foltz wrote: > Add a brief comment describing the control flow and loop invariant. Done. https://codereview.chromium.org/79673003/diff/370001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:590: DoConnectLoop(net::OK); On 2013/12/03 00:56:00, mark a. foltz wrote: > So the flow here is DoConnectLoop -> DoAuthChallengeSendComplete -> post task to > StartReadLoop -> DoReadLoop -> DoReadComplete -> ProcessBody -> > ParseMessageFromBody -> DoConnectLoop (having advanced the state to > CONN_STATE_AUTH_CHALLENGE_REPLY_COMPLETE). > > To eliminate the PostTask, we could terminate the first invocation of the > DoReadLoop() terminate on receiving the auth reply and return up to > DoConnectLoop(), and later restart the read loop after the connection completes. > I am not sure if that improves the code or not though. I am trying an approach to either remove the need for PostTask completely or to only do it when read finishes synchronously. Let us see how it goes.
Ih av e uploaded another patch. Please take alook. Comments about various discussions: - There is a trade-off between code complexity and performance. We have a lot of options on where to set that trade-off. - One extreme is to do most performant code by avoiding PostTask completely and introducing more complexity in the code. That makes the code harder to understand and verify. - Other extreme is to always do PostTask before invoking callbacks. But in this case we will also do PostTask for every read/write, not just during connection flow. - There are options in the middle as well. - A big value of CastSocket class is to convert semantics of net/ style code of using return value and callbacks together to report results to always reporting results via callbacks in this class. Given that we only need to mix DoConnectLoop with DoWriteLoop and DoReadLoop in the connection flow, performance is not as critical. So I have chosen the following approach: - During connection flow, post a task to start write flow and read flow. This means DoWriteLoop and DoReadLoop are not nested inside DoConnectLoop. So the code in write flow nad read flow are reasonably decoupled from connect flow. There is no other PostTask apart from the above.
I mis-spoke a bit in my last message. I chose an approahc where we never intermix DoConnectLoop with DoReadLoop or DoWriteLoop on the stack. That is there will not be two Do*Loop methods on the stack at the same time. This was the easiest way to make sure we have no user-after-free bugs in case the connection callback ends up deleting the socket.
Looks good other than a few comments. After the changes we discussed offline, needs unit tests for a LGTM. https://codereview.chromium.org/79673003/diff/470001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/470001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:241: //// bool success = tcp_socket_->SetKeepAlive(true, kTcpKeepAliveDelaySecs); Should these lines be commented out? https://codereview.chromium.org/79673003/diff/470001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:577: // If the message is an auth message then we handle it internally. I feel that we should check connect_state_ here to ensure we are actually inside the connect loop. If the other side sends an auth reply message after we've finished the connect loop (for whatever reason) we'll be in a weird flow where we attempt to revalidate the message and fire the connect callback again. If we're not in the connect loop I would CloseWithError() with a socket error. https://codereview.chromium.org/79673003/diff/470001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/79673003/diff/470001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:108: // fine to delete the socket in Delegate::ONError Typo in Delegate:OnError
undo LGTM Sigh...
not LGTM
Done. I am workingo n unit tests. https://codereview.chromium.org/79673003/diff/470001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/470001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:241: //// bool success = tcp_socket_->SetKeepAlive(true, kTcpKeepAliveDelaySecs); On 2013/12/05 23:42:49, mark a. foltz wrote: > Should these lines be commented out? Done. https://codereview.chromium.org/79673003/diff/470001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:577: // If the message is an auth message then we handle it internally. On 2013/12/05 23:42:49, mark a. foltz wrote: > I feel that we should check connect_state_ here to ensure we are actually inside > the connect loop. If the other side sends an auth reply message after we've > finished the connect loop (for whatever reason) we'll be in a weird flow where > we attempt to revalidate the message and fire the connect callback again. > > If we're not in the connect loop I would CloseWithError() with a socket error. Done. Good call.
https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:172: DoConnectLoop(net::OK); DESIGN nit: the net/ idiom is as follows This becomes ready_state_ = READY_STATE_CONNECTING; connect_state_ = CONN_STATE_TCP_CONNECT; int rv = DoConnectLoop(net::OK); if (rv == net::ERR_IO_PENDING) { connect_callback_ = callback; return; } // Note: |callback| may delete |this| callback.Run(rv); https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:175: void CastSocket::PostTaskToStartConnectLoop(int result) { DESIGN NIT: Rather than have DoConnectLoop invoke the callback, this is traditionally done by something as follows void CastSocket::RestartConnectLoop(int result) { DCHECK(CalledOnValidThread()); base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(&CastSocket::OnConnectIOComplete, ...))) } void CastSocket::OnConnectIOComplete(int result) { int rv = DoConnectLoop(result); if (rv == net::ERR_IO_PENDING) return; // Note: |connect_callback_| may cause |this| to be deleted. base::ResetAndReturn(&connect_callback_).Run(result); }; The goal with this paradigm is to make it absolutely clear where the exit points are in terms of ownership (that is, when |callback| will be run, and when it may delete |this|). The current implementation throws that ownership model throughout the code. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:196: // method changed state or not. comment nit: This makes the comment describe implementation details, rather than goals. // Default to CONN_STATE_NONE, which break the processing loop if any // handler fails to transition to another state to continue processing. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:232: // callback in either case. comment nit: pronouns are considered harmful - https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... That said, this all seems like it shouldn't be necessary. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:284: // code decoupled from connect loop code. This is really unfortunate that you're forcing always-async. this has a very real cost on Android (~5ms in worst case). Having the code clearly structured on ownership lets you have a smooth synchronous implementation. I still need to page all of this code into make a solid recommendation, but given that we've accomplished it for SPDY and QUIC (for example), I feel like we should be able to do the same here. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:290: // Always return IO_PENDING since we always get the result asynchronously comment nits re: pronouns apply throughout this file. See the thread for easy and simple ways to reword comments without pronouns to be clearer and less ambiguous. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:329: tcp_socket_.reset(NULL); Just .reset() should be sufficient for all of these, and is idiomatic. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:334: callback.Run(net::OK); |callback| can delete |this|, correct? Seems like you should add a comment here to that effect. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:408: CloseWithError(error_state_); |this| can be deleted, can't it? Subtle. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:51: public base::SupportsWeakPtr<CastSocket> { See https://groups.google.com/a/chromium.org/d/msg/chromium-dev/N6w_CkKEwvE/evs2p... SupportsWeakPtr is discouraged https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:57: // It is fine to delete the socket in this callback. Design nit: Within net/, the delegates tend not to take the individual socket. It's typically up to the "embedder" to handle memoization. This is important, because as this code is currently written, it creates an ownership ambiguity situation that should not exist. The CastSocket does not own itself, but it "implies" that by passing itself to the Delegate and suggesting it's ok to delete. Instead, we make Delegates handle memoization (eg: OnError(ChannelError error) / OnMessage(const MessageInfo& message) ). This makes it clear it's the Delegate's responsibility to handle ownership and lifetime responsibly. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:109: // But it is fine to delete the socket in Delegate::ONError Typo (ONError = OnError) https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:121: protected: Why are these methods protected? Seems like they should be private. Am I missing something? https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:210: // write state. For e.g. when write state is WRITE_STATE_WRITE_COMPLETE For example or e.g. , but not For e.g. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:240: // Posts a task tostart the connect loop with the given |result|. typo: tostart -> to start That said, these comments seem unnecessary per the style guide (same on 242) https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:254: void CloseWithError(ChannelError error); comment nit: // Closes socket, updating the error state and signaling the delegate that |error| has occurred. Design Nit: By having this function responsible for signaling the delegate_, it makes it hard to at a glance verify that the lifetime semantics are being observed (namely, that the object may be deleted by the return of this function).
https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:172: DoConnectLoop(net::OK); On 2013/12/11 08:14:14, Ryan Sleevi wrote: > DESIGN nit: the net/ idiom is as follows > > This becomes > > ready_state_ = READY_STATE_CONNECTING; > connect_state_ = CONN_STATE_TCP_CONNECT; > int rv = DoConnectLoop(net::OK); > if (rv == net::ERR_IO_PENDING) { > connect_callback_ = callback; > return; > } > > // Note: |callback| may delete |this| > callback.Run(rv); Yes. I changed it todo return void and do thecallback as part of the loop so that multiple places don't have to remember to do the above. This is also useful since connect loop can start write loop or read loop and can expect a callback into it without the code calling it back doing the aboe (because DoConnectLoop is now void). https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:175: void CastSocket::PostTaskToStartConnectLoop(int result) { On 2013/12/11 08:14:14, Ryan Sleevi wrote: > DESIGN NIT: Rather than have DoConnectLoop invoke the callback, this is > traditionally done by something as follows > > void CastSocket::RestartConnectLoop(int result) { > DCHECK(CalledOnValidThread()); > base::MessageLoop::current()->PostTask(FROM_HERE, > base::Bind(&CastSocket::OnConnectIOComplete, ...))) > } > > void CastSocket::OnConnectIOComplete(int result) { > int rv = DoConnectLoop(result); > if (rv == net::ERR_IO_PENDING) > return; > > // Note: |connect_callback_| may cause |this| to be deleted. > base::ResetAndReturn(&connect_callback_).Run(result); > }; > > > The goal with this paradigm is to make it absolutely clear where the exit points > are in terms of ownership (that is, when |callback| will be run, and when it may > delete |this|). The current implementation throws that ownership model > throughout the code. The current code only has one place where the callback is performed: DoConnectCallback. It also has only one point of entry - DoConnectLoop. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:196: // method changed state or not. On 2013/12/11 08:14:14, Ryan Sleevi wrote: > comment nit: This makes the comment describe implementation details, rather than > goals. > > // Default to CONN_STATE_NONE, which break the processing loop if any > // handler fails to transition to another state to continue processing. Done. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:232: // callback in either case. On 2013/12/11 08:14:14, Ryan Sleevi wrote: > comment nit: pronouns are considered harmful - > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... > > That said, this all seems like it shouldn't be necessary. Done. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:284: // code decoupled from connect loop code. On 2013/12/11 08:14:14, Ryan Sleevi wrote: > This is really unfortunate that you're forcing always-async. this has a very > real cost on Android (~5ms in worst case). > > Having the code clearly structured on ownership lets you have a smooth > synchronous implementation. I still need to page all of this code into make a > solid recommendation, but given that we've accomplished it for SPDY and QUIC > (for example), I feel like we should be able to do the same here. See my overall comment on the patch that addresses the various comments regarding design. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:290: // Always return IO_PENDING since we always get the result asynchronously On 2013/12/11 08:14:14, Ryan Sleevi wrote: > comment nits re: pronouns apply throughout this file. See the thread for easy > and simple ways to reword comments without pronouns to be clearer and less > ambiguous. Done. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:329: tcp_socket_.reset(NULL); On 2013/12/11 08:14:14, Ryan Sleevi wrote: > Just .reset() should be sufficient for all of these, and is idiomatic. Done. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:334: callback.Run(net::OK); On 2013/12/11 08:14:14, Ryan Sleevi wrote: > |callback| can delete |this|, correct? > > Seems like you should add a comment here to that effect. Done. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:51: public base::SupportsWeakPtr<CastSocket> { On 2013/12/11 08:14:14, Ryan Sleevi wrote: > See > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/N6w_CkKEwvE/evs2p... > > SupportsWeakPtr is discouraged Yes. I will work on removing that in a separate patch. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:57: // It is fine to delete the socket in this callback. On 2013/12/11 08:14:14, Ryan Sleevi wrote: > Design nit: Within net/, the delegates tend not to take the individual socket. > It's typically up to the "embedder" to handle memoization. > > This is important, because as this code is currently written, it creates an > ownership ambiguity situation that should not exist. The CastSocket does not own > itself, but it "implies" that by passing itself to the Delegate and suggesting > it's ok to delete. > > Instead, we make Delegates handle memoization (eg: OnError(ChannelError error) / > OnMessage(const MessageInfo& message) ). This makes it clear it's the Delegate's > responsibility to handle ownership and lifetime responsibly. I agree with your feedback. Since this change requires chagnes to the embedder, I will make this change separately. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:109: // But it is fine to delete the socket in Delegate::ONError On 2013/12/11 08:14:14, Ryan Sleevi wrote: > Typo (ONError = OnError) Done. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:121: protected: On 2013/12/11 08:14:14, Ryan Sleevi wrote: > Why are these methods protected? > > Seems like they should be private. Am I missing something? They are overridden in tests to inject mock results. But changed CalledOnValidThread to be private. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:210: // write state. For e.g. when write state is WRITE_STATE_WRITE_COMPLETE On 2013/12/11 08:14:14, Ryan Sleevi wrote: > For example or e.g. , but not For e.g. Done. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:240: // Posts a task tostart the connect loop with the given |result|. On 2013/12/11 08:14:14, Ryan Sleevi wrote: > typo: tostart -> to start > > That said, these comments seem unnecessary per the style guide (same on 242) Done. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:254: void CloseWithError(ChannelError error); On 2013/12/11 08:14:14, Ryan Sleevi wrote: > comment nit: > > // Closes socket, updating the error state and signaling the delegate that > |error| has occurred. > > Design Nit: By having this function responsible for signaling the delegate_, it > makes it hard to at a glance verify that the lifetime semantics are being > observed (namely, that the object may be deleted by the return of this > function). Done. Once I make the change you suggest of removing the socket paramter from the delegate, your (valid) comment here will be addressed automatically.
Unit tests refactoring is also done now and the patch is fully ready for final review. Please take a look
It's definitely going to take me a while to read through your unit tests. They're very unlike any other code we have. Some initial feedback though. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:175: void CastSocket::PostTaskToStartConnectLoop(int result) { On 2013/12/11 23:39:41, Munjal (Google) wrote: > On 2013/12/11 08:14:14, Ryan Sleevi wrote: > > DESIGN NIT: Rather than have DoConnectLoop invoke the callback, this is > > traditionally done by something as follows > > > > void CastSocket::RestartConnectLoop(int result) { > > DCHECK(CalledOnValidThread()); > > base::MessageLoop::current()->PostTask(FROM_HERE, > > base::Bind(&CastSocket::OnConnectIOComplete, ...))) > > } > > > > void CastSocket::OnConnectIOComplete(int result) { > > int rv = DoConnectLoop(result); > > if (rv == net::ERR_IO_PENDING) > > return; > > > > // Note: |connect_callback_| may cause |this| to be deleted. > > base::ResetAndReturn(&connect_callback_).Run(result); > > }; > > > > > > The goal with this paradigm is to make it absolutely clear where the exit > points > > are in terms of ownership (that is, when |callback| will be run, and when it > may > > delete |this|). The current implementation throws that ownership model > > throughout the code. > > The current code only has one place where the callback is performed: > DoConnectCallback. It also has only one point of entry - DoConnectLoop. I apologize if my comment before was not clearer. DoConnectLoop is not the only point of entry. Any public API that causes DoConnectLoop to be called is the point of entry. My goal in the above suggestion was to make sure that the callback invocation was not buried in the details of the implementation, but as close to possible of the actual public API entry points, or the start of a task. I still think the way you've altered the code is dangerous and error-prone, even if (probably) correct. https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:121: protected: On 2013/12/11 23:39:41, Munjal (Google) wrote: > On 2013/12/11 08:14:14, Ryan Sleevi wrote: > > Why are these methods protected? > > > > Seems like they should be private. Am I missing something? > > They are overridden in tests to inject mock results. > If they are overridden in tests, they can be private but virtual. There's no need to make them protected-virtual if your goal is to have your mock stub in results. > But changed CalledOnValidThread to be private. https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:406: // state to NONE pronoun comment https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:665: std::string tmp = CastMessageToString(message_proto); Unnecessary? https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:207: // write state. For e.g. when write state is WRITE_STATE_WRITE_COMPLETE Still says "For e.g." (and there are multiple occurrences in this file) https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:253: // Returns whether we are executing in a valid thread. You introduced another pronoun comment. It's also not clear why this is virtual. And it's just a forward to the ThreadChecker, is it not? https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:70: // Always return true in tests These comments provide zero value. https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:89: } This behaviour for Read and Write leaves me very uncomfortable with the actual coverage quality of the unittests. Supporting the Mock Sockets is highly desirable here so you can actually test your assumptinos about the underlying sockets behaviour.
https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/510001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:175: void CastSocket::PostTaskToStartConnectLoop(int result) { On 2013/12/12 00:48:14, Ryan Sleevi wrote: > On 2013/12/11 23:39:41, Munjal (Google) wrote: > > On 2013/12/11 08:14:14, Ryan Sleevi wrote: > > > DESIGN NIT: Rather than have DoConnectLoop invoke the callback, this is > > > traditionally done by something as follows > > > > > > void CastSocket::RestartConnectLoop(int result) { > > > DCHECK(CalledOnValidThread()); > > > base::MessageLoop::current()->PostTask(FROM_HERE, > > > base::Bind(&CastSocket::OnConnectIOComplete, ...))) > > > } > > > > > > void CastSocket::OnConnectIOComplete(int result) { > > > int rv = DoConnectLoop(result); > > > if (rv == net::ERR_IO_PENDING) > > > return; > > > > > > // Note: |connect_callback_| may cause |this| to be deleted. > > > base::ResetAndReturn(&connect_callback_).Run(result); > > > }; > > > > > > > > > The goal with this paradigm is to make it absolutely clear where the exit > > points > > > are in terms of ownership (that is, when |callback| will be run, and when it > > may > > > delete |this|). The current implementation throws that ownership model > > > throughout the code. > > > > The current code only has one place where the callback is performed: > > DoConnectCallback. It also has only one point of entry - DoConnectLoop. > > I apologize if my comment before was not clearer. > > DoConnectLoop is not the only point of entry. > > Any public API that causes DoConnectLoop to be called is the point of entry. My > goal in the above suggestion was to make sure that the callback invocation was > not buried in the details of the implementation, but as close to possible of the > actual public API entry points, or the start of a task. > > I still think the way you've altered the code is dangerous and error-prone, even > if (probably) correct. There is only one public method that starts the connect loop - Connect. I understand your point - in the case where the caller of DoConnectLoop calls the callback, DoConnectLoop won't be on teh stack when callback is called. But the downside of that approach is that there are multiple places which have to duplicate the code snippet you pasted above. If we create a helper to do that code snippet, then we have the same issue of code-calling-callback-buried-deeper. So I slightly prefer the approach of doing the callback in the connect flow itself. https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:207: // write state. For e.g. when write state is WRITE_STATE_WRITE_COMPLETE On 2013/12/12 00:48:14, Ryan Sleevi wrote: > Still says "For e.g." (and there are multiple occurrences in this file) Done. https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.h:253: // Returns whether we are executing in a valid thread. On 2013/12/12 00:48:14, Ryan Sleevi wrote: > You introduced another pronoun comment. > > It's also not clear why this is virtual. And it's just a forward to the > ThreadChecker, is it not? - Removed comment since it felt redundant anyway. - virtual is a residue from something I was trying out. Removed now. - Yes, it is a forward to the thread checker. https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:70: // Always return true in tests On 2013/12/12 00:48:14, Ryan Sleevi wrote: > These comments provide zero value. The reason I have these comments is so that the reader knows that the behavior is intended. I find such comments useful in situations when the reader is not sure if the person who wrote the code has made a mistake or is it intentional. https://codereview.chromium.org/79673003/diff/530001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:89: } On 2013/12/12 00:48:14, Ryan Sleevi wrote: > This behaviour for Read and Write leaves me very uncomfortable with the actual > coverage quality of the unittests. Supporting the Mock Sockets is highly > desirable here so you can actually test your assumptinos about the underlying > sockets behaviour. Actually the Read/Write methods of the TCP socket should never be called. I changed the mock impl to reflect that.
Tests look good overall. It looks like the read and write tests in particular share a lot of code and could benefit from a base class. Subclassing could permit even further coverage, such as alternating sync/async reads/writes. Obviously there is a limit to the number of configurations though. Some of the message construction and parsing routines that are shared with the CastSocket implementation belong in cast_message_util, as long as an API that permits the extra conversions to/from an IOBuffer for the real socket can be devised. The net:: mocks add some unfortunate complexity to the unit test. Fortunately it's encapsulated in the helper classes and the test cases remain relatively clean. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:70: virtual bool SetKeepAlive(bool a, int b) OVERRIDE { Please preserve parameter names from the original declaration so the reader can tell what they mean, without having to hunt for that declaration. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:75: virtual bool SetNoDelay(bool b) OVERRIDE { Ditto. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:178: ssl_data_.reset(new net::StaticSocketDataProvider( This is a weird API. - Why doesn't the SSDP have reads_ and writes_ members? - Why is it taking a pointer to the vectors' internal array? This violates abstraction and creates shared ownership that's probably not necessary. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:182: new net::MockTCPClientSocket( Did you mean MockTCPSocket? https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:200: scoped_ptr<net::MockConnect> ssl_connect_data_[2]; It's unfortunate the use of the net:: mocks requires all this extra bookkeeping (in essence this is reinventing GMock). https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:459: size_t body_size = auth_reply_.length() - 4; Please add a utility function for this expression that is repeated several times below. Would also prefer that you use the defined constant for the header size. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:571: // Test connectiion error - challenge send fails connection, here and several places below. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:629: // Test write success - single message (async) The Write tests all share similar code to set up the test case. Consider creating a subclass of CastSocketTest and factoring this code into its Setup() method. For bonus points you could move the test cases themselves into a base class and have two subclasses (CastSocketAsyncWriteTest, CastSocketSyncWriteTest) that pass the corresponding flag into the mock socket to test asynchronous or synchronous completion. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:775: new base::StringValue(std::string(65537, 'a'))); FYI, the maximum length allowed for a message is 64KB. This is enforced at the API layer though so it's okay to use a larger message here. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:839: Extra newlines https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:867: CreateCastSocket(); Similar comments about creating a base class to hold the content of the read test cases, with subclasses to handle the async vs. sync cases. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:1021: memcpy(&header, &body_size, arraysize(header)); There shouldn't be a need to duplicate the code to construct a message header + payload between here, below and in cast_socket.cc. Please add a TODO to factor out a utility method into cast_message_util.cc.
PTAL. I have factored out most of the duplicate code from tests into helpers. Like we discussed offline, base class approach did not feel appropriate to me given that the tests are now quite readable: create target, set expectations, call a public method, verify results. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:70: virtual bool SetKeepAlive(bool a, int b) OVERRIDE { On 2013/12/13 01:02:02, mark a. foltz wrote: > Please preserve parameter names from the original declaration so the reader can > tell what they mean, without having to hunt for that declaration. Done. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:75: virtual bool SetNoDelay(bool b) OVERRIDE { On 2013/12/13 01:02:02, mark a. foltz wrote: > Ditto. Done. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:178: ssl_data_.reset(new net::StaticSocketDataProvider( On 2013/12/13 01:02:02, mark a. foltz wrote: > This is a weird API. > > - Why doesn't the SSDP have reads_ and writes_ members? > - Why is it taking a pointer to the vectors' internal array? This violates > abstraction and creates shared ownership that's probably not necessary. > SSDP requires array of reads and writes in the ctor. It does not have any public methods to add those later since (I think) it is an array internally. The reason we use a vector is that SSDP does not take ownership of the arrays we pass in. It wants us to keep the ownership. So we have ot anyway use some data structure to deallcoate those arrays after tests are done. I chose vector instead fo scoped_array so that the tests get semantics of AddXResult(...) instead of having to pass in all the results at once. This makes the tests more readable. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:182: new net::MockTCPClientSocket( On 2013/12/13 01:02:02, mark a. foltz wrote: > Did you mean MockTCPSocket? No. net::MockTCPSocket (in socket_test_util.h/.cc) inherits from net::SSLClientSocket. It is bad naming but that is how it is. I added a comment to that regard though to make it clear that it is intentional. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:200: scoped_ptr<net::MockConnect> ssl_connect_data_[2]; On 2013/12/13 01:02:02, mark a. foltz wrote: > It's unfortunate the use of the net:: mocks requires all this extra bookkeeping > (in essence this is reinventing GMock). Actually this particular array of two sockets is not due totheuse of net:: mocks. This is to avoid the weird ownership model in the old code where it kept a flag (like owns_ssl_socket_) to figure out whether we own the socket or not. The new code fixes that because we don't need to hold on to the socket pointer to set expectations on it or invoke callbacks. So in that regard the new approach is better. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:459: size_t body_size = auth_reply_.length() - 4; On 2013/12/13 01:02:02, mark a. foltz wrote: > Please add a utility function for this expression that is repeated several times > below. > > Would also prefer that you use the defined constant for the header size. Done. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:571: // Test connectiion error - challenge send fails On 2013/12/13 01:02:02, mark a. foltz wrote: > connection, here and several places below. Done. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:629: // Test write success - single message (async) On 2013/12/13 01:02:02, mark a. foltz wrote: > The Write tests all share similar code to set up the test case. > Consider creating a subclass of CastSocketTest and factoring this code into its > Setup() method. > > For bonus points you could move the test cases themselves into a base class and > have two subclasses (CastSocketAsyncWriteTest, CastSocketSyncWriteTest) that > pass the corresponding flag into the mock socket to test asynchronous or > synchronous completion. Are you talking about the calls to AddWriteResult by "setup code"? The reason I like the current style is that the test is mostly self-contained and readable. It is easy to figure out what the test is doing. If we abstract too many things out of the test it will compromise that readability. Having said that I added a couple of more helpers to more easily add write and read results to the mock socket. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:775: new base::StringValue(std::string(65537, 'a'))); On 2013/12/13 01:02:02, mark a. foltz wrote: > FYI, the maximum length allowed for a message is 64KB. > > This is enforced at the API layer though so it's okay to use a larger message > here. Yeah, this is to test the case when the message is too large. It helps us test one more code path in the implementation. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:839: On 2013/12/13 01:02:02, mark a. foltz wrote: > Extra newlines Done. https://codereview.chromium.org/79673003/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:1021: memcpy(&header, &body_size, arraysize(header)); On 2013/12/13 01:02:02, mark a. foltz wrote: > There shouldn't be a need to duplicate the code to construct a message header + > payload between here, below and in cast_socket.cc. Please add a TODO to factor > out a utility method into cast_message_util.cc. Done.
Mark, can you take another look?
On 2013/12/17 19:11:54, Munjal (Google) wrote: > Mark, can you take another look? LGTM ! Thanks, that addresses my concerns with the code redundancy in the tests. A few minor nits.
https://codereview.chromium.org/79673003/diff/720001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/79673003/diff/720001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:45: static void CreateStringMessage(MessageInfo* msg, Nit: please name parameters consistently with the rest of the code base and declaration of the cast.channel API, so: msg -> message ns -> namespace_ src -> source_id dst -> destination_id https://codereview.chromium.org/79673003/diff/720001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:45: static void CreateStringMessage(MessageInfo* msg, Output parameters usually go at the end of the parameter list. https://codereview.chromium.org/79673003/diff/720001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:46: std::string ns, const std::string& here and below
https://codereview.chromium.org/79673003/diff/720001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/79673003/diff/720001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:45: static void CreateStringMessage(MessageInfo* msg, On 2013/12/17 23:31:03, mark a. foltz wrote: > Nit: please name parameters consistently with the rest of the code base and > declaration of the cast.channel API, so: > > msg -> message > ns -> namespace_ > src -> source_id > dst -> destination_id Done. https://codereview.chromium.org/79673003/diff/720001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:45: static void CreateStringMessage(MessageInfo* msg, On 2013/12/17 23:31:03, mark a. foltz wrote: > Output parameters usually go at the end of the parameter list. Done. https://codereview.chromium.org/79673003/diff/720001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket_unittest.cc:46: std::string ns, On 2013/12/17 23:31:03, mark a. foltz wrote: > const std::string& here and below Done.
https://codereview.chromium.org/79673003/diff/750001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/750001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:65: const uint32 kMessageHeaderSize = sizeof(uint32); Why would you take sizeof(uint32)? You're saying it's 32-bit, why bother with sizeof? :)
On 2013/12/18 00:02:13, Ryan Sleevi wrote: > https://codereview.chromium.org/79673003/diff/750001/chrome/browser/extension... > File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): > > https://codereview.chromium.org/79673003/diff/750001/chrome/browser/extension... > chrome/browser/extensions/api/cast_channel/cast_socket.cc:65: const uint32 > kMessageHeaderSize = sizeof(uint32); > Why would you take sizeof(uint32)? You're saying it's 32-bit, why bother with > sizeof? :) Defensive coding against the possibility of a future high-bits-per-byte architecture that stores two physical bits for every one logical one?
On 2013/12/18 00:48:44, Wez wrote: > On 2013/12/18 00:02:13, Ryan Sleevi wrote: > > > https://codereview.chromium.org/79673003/diff/750001/chrome/browser/extension... > > File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): > > > > > https://codereview.chromium.org/79673003/diff/750001/chrome/browser/extension... > > chrome/browser/extensions/api/cast_channel/cast_socket.cc:65: const uint32 > > kMessageHeaderSize = sizeof(uint32); > > Why would you take sizeof(uint32)? You're saying it's 32-bit, why bother with > > sizeof? :) > > Defensive coding against the possibility of a future high-bits-per-byte > architecture that stores two physical bits for every one logical one? But it would still be serialized on the wire as 4 bytes - since it needs to be network operable. The size is the measure of how much memory is needed in the (serialized) message.
https://codereview.chromium.org/79673003/diff/750001/chrome/browser/extension... File chrome/browser/extensions/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/79673003/diff/750001/chrome/browser/extension... chrome/browser/extensions/api/cast_channel/cast_socket.cc:65: const uint32 kMessageHeaderSize = sizeof(uint32); On 2013/12/18 00:02:13, Ryan Sleevi wrote: > Why would you take sizeof(uint32)? You're saying it's 32-bit, why bother with > sizeof? :) The idea is that(originally by mfoltz) kMessageHeaderSize should be sum of sizeof of various fields in MessageHeader struct. Currently there is only one uint32 field. But if we add more fields we will update this constant as well.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Rubber-stamp LGTM based on mfoltz approval.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/munjal@chromium.org/79673003/750001
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/munjal@chromium.org/79673003/750001
Message was sent while issue was closed.
Change committed as 242158 |
