Changed MessageReader so that it doesn't read from the socket if there are

Changed MessageReader so that it doesn't read from the socket if there are
other messages being processed. Added unittests for MessageReader.

BUG=None
TEST=Unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72262

[awong, 2011-01-19 02:12:48]

http://codereview.chromium.org/6271004/diff/3001/remoting/protocol/ref_counte...
File remoting/protocol/ref_counted_task.h (right):

http://codereview.chromium.org/6271004/diff/3001/remoting/protocol/ref_counte...
remoting/protocol/ref_counted_task.h:23: class RefCountedTask : public
base::RefCounted<RefCountedTask> {
I'm not sure I completely agree with having this class.  If we're doing this, it
feels like we should be passing around a refcounted object.

Is there a way to join and/or cancel all the outstanding requests?  Maybe using
a ScopedRunnableMethodFactory or something?

[Sergey Ulanov, 2011-01-19 20:07:00]

http://codereview.chromium.org/6271004/diff/3001/remoting/protocol/ref_counte...
File remoting/protocol/ref_counted_task.h (right):

http://codereview.chromium.org/6271004/diff/3001/remoting/protocol/ref_counte...
remoting/protocol/ref_counted_task.h:23: class RefCountedTask : public
base::RefCounted<RefCountedTask> {
On 2011/01/19 02:12:48, awong wrote:
> I'm not sure I completely agree with having this class.  If we're doing this,
it
> feels like we should be passing around a refcounted object.
> 
> Is there a way to join and/or cancel all the outstanding requests?  Maybe
using
> a ScopedRunnableMethodFactory or something?

ScopedRunnableMethodFactory can't be used here because it is not thread-safe,
and I don't see how it can be applicable here (or was your comment about
MessageReader::DoneTaskHandler?).

This class is just used to call a task ref count gets to zero, and then deletes
that task..

[Sergey Ulanov, 2011-01-19 22:15:58]

I've removed compound messages and RefCountedTask class.

[awong, 2011-01-20 20:06:37]

Thanks for cleaning that up.  It does indeed look much simpler.

http://codereview.chromium.org/6271004/diff/13001/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/proto/event.proto#...
remoting/proto/event.proto:45: message EventMessage {
Nit, I would just called his "event" or better "InputEvent"

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
File remoting/protocol/client_message_dispatcher.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
remoting/protocol/client_message_dispatcher.cc:49: LOG(ERROR) << "Invalid
control message received.";
LOG(WARNING) please.

Let's try to keep LOG(ERROR) for events that are more like "omg! no clue what is
happening!  I'd crash right now, but I can still limp along...maybe"  kind of
states. :)

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/fake_sess...
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/fake_sess...
remoting/protocol/fake_session.h:33: int input_pos() { return input_pos_; }
const functions here and below?

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_d...
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_d...
remoting/protocol/message_decoder.h:40: // messages yet.
Should we note somoewhere that each CompoundBuffer is guaranteed to only have 1
message?  Should there be a dcheck and/or unittest to verify this assertion
remains true?

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_d...
File remoting/protocol/message_decoder_unittest.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_d...
remoting/protocol/message_decoder_unittest.cc:89: it != message_list.end();
++it) {
Can you add a SCOPED_LOG in this, and the other for loops in this unittest file
to make sure we know which iteration we're on (or whatever other information is
required to pinpoint the failing input)?

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
File remoting/protocol/message_reader.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.cc:35: void OnDone(CompoundBuffer* buffer) {
2 thoughts:
  1) I wonder if we shouldn't delete the buffer back on the jingle_thread. 
Feels slightly cleaner, and we can collapse OnDone with ProcessDoneEvent.

  2)  Can't we replace this class with a WeakPtr & funciton?

Example:

  static void MessageDone(MessageLoop* run_on_me, WeakPtr<MessageReader>
message_reader, CompoundBuffer *b) {
   if (MessageLoop::current() != run_on_me) {
       // post back to self..
   }

   delete b;
   if (message_reader) {
     message_reader->OnMessageDone();
   }
}

?

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
File remoting/protocol/message_reader.h (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.h:25: // callback for each message it receives
We should document the flow control semantics here.  In particular, when do we
enter "read_pending", and how is the client expected to respond?

I also wonder if we shouldn't just only expose pending_messages, and tell the
client to throttle based on that rather than whether or not a read itself is
oustanding...  We might even list outstanding read requests as
"pending_messages" from the client perspective...

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.h:31: // argument is called.
Nice comment!

Thanks for being disciplined with documenting ownership semantics.

Ewww still about the 2 argument callback, but what can you do :(

[Alpha Left Google, 2011-01-20 20:54:51]

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
File remoting/protocol/client_message_dispatcher.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
remoting/protocol/client_message_dispatcher.cc:49: LOG(ERROR) << "Invalid
control message received.";
On 2011/01/20 20:06:38, awong wrote:
> LOG(WARNING) please.
> 
> Let's try to keep LOG(ERROR) for events that are more like "omg! no clue what
is
> happening!  I'd crash right now, but I can still limp along...maybe"  kind of
> states. :)

If it's something really unexpected like this then NOTREACHED is better, it's
bad enough to receive a corrupted message.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/host_mess...
File remoting/protocol/host_message_dispatcher.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/host_mess...
remoting/protocol/host_message_dispatcher.cc:57: LOG(ERROR) << "Invalid control
message received.";
Same here NOTREACHED is better, we should be more strict about messages in the
channel.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_d...
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_d...
remoting/protocol/message_decoder.h:40: // messages yet.
On 2011/01/20 20:06:38, awong wrote:
> Should we note somoewhere that each CompoundBuffer is guaranteed to only have
1
> message?  Should there be a dcheck and/or unittest to verify this assertion
> remains true?

I don't think there's a simple DCHECK who can do this... :( It really relies how
we craft the compoundbuffer in this class.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
File remoting/protocol/message_reader.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.cc:91: while (!closed_ && !read_pending_ &&
pending_messages_ == 0) {
The flag of |read_pending| is confusing to me. According to the code we never
issue two DoRead() at the same time. Also we should never read when there's
outstanding DoneTaskHandler running, which means when pending_messages > 0,
read_pending is never true. What I'm saying is that putting !read_pending_ in
the condition is hiding the problem (which i believe the current code has, see
my comments below..). Either |read_pending_| should be removed or used as a
DCHECK to make sure it's exclusive with pending_messages >0.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.cc:131: pending_messages_++;
I think there's a bug here. Incrementing here will not do what you intended to.
If the following Run() calls trigger OnMessageDone() then pending_messages will
immediately become 0. Either you always post a task in ProcessDoneEvent() or
change this to become:

vector<pair<CompoundBuffer*, Task*> > invocations;

while (true) {
  CompoundBuffer* buffer = message_decoder_.GetNextMessage();
  if (!buffer) break;
}

pending_messages_ += invocations.size();

while (invocations...) {
  message_received_callback_->Run(....);
}

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
File remoting/protocol/message_reader.h (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.h:53: bool read_pending_;
Please add some comments for these variables, it will be very helpful to
understand how the flow control works.

[awong, 2011-01-20 21:01:56]

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
File remoting/protocol/client_message_dispatcher.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
remoting/protocol/client_message_dispatcher.cc:49: LOG(ERROR) << "Invalid
control message received.";
On 2011/01/20 20:54:52, Alpha wrote:
> On 2011/01/20 20:06:38, awong wrote:
> > LOG(WARNING) please.
> > 
> > Let's try to keep LOG(ERROR) for events that are more like "omg! no clue
what
> is
> > happening!  I'd crash right now, but I can still limp along...maybe"  kind
of
> > states. :)
> 
> If it's something really unexpected like this then NOTREACHED is better, it's
> bad enough to receive a corrupted message.

Is the API contract that the control message is valid before being passed to
this function?  If so, then I agree with alpha that NOTREACHED is better.

Then, I would also expect an "IsValidControlMessage" function that we can
unittest, and which guards the callpoint to this handler.

[Sergey Ulanov, 2011-01-20 21:55:56]

http://codereview.chromium.org/6271004/diff/13001/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/proto/event.proto#...
remoting/proto/event.proto:45: message EventMessage {
On 2011/01/20 20:06:38, awong wrote:
> Nit, I would just called his "event" or better "InputEvent"
I think it is better to call it EventMessage for consistency with
ControlMessage. Moved this to internal.proto.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
File remoting/protocol/client_message_dispatcher.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
remoting/protocol/client_message_dispatcher.cc:49: LOG(ERROR) << "Invalid
control message received.";
On 2011/01/20 20:06:38, awong wrote:
> LOG(WARNING) please.
> 
> Let's try to keep LOG(ERROR) for events that are more like "omg! no clue what
is
> happening!  I'd crash right now, but I can still limp along...maybe"  kind of
> states. :)

Done.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
remoting/protocol/client_message_dispatcher.cc:49: LOG(ERROR) << "Invalid
control message received.";
On 2011/01/20 20:54:52, Alpha wrote:
> On 2011/01/20 20:06:38, awong wrote:
> > LOG(WARNING) please.
> > 
> > Let's try to keep LOG(ERROR) for events that are more like "omg! no clue
what
> is
> > happening!  I'd crash right now, but I can still limp along...maybe"  kind
of
> > states. :)
> 
> If it's something really unexpected like this then NOTREACHED is better, it's
> bad enough to receive a corrupted message.
If you put NOTREACHED here, it will be very easy to DoS a host: just send an
invalid message to it.
|message| is received from the network, so we don't have any guarantees about it
whatsoever.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
remoting/protocol/client_message_dispatcher.cc:49: LOG(ERROR) << "Invalid
control message received.";
On 2011/01/20 21:01:56, awong wrote:
> On 2011/01/20 20:54:52, Alpha wrote:
> > On 2011/01/20 20:06:38, awong wrote:
> > > LOG(WARNING) please.
> > > 
> > > Let's try to keep LOG(ERROR) for events that are more like "omg! no clue
> what
> > is
> > > happening!  I'd crash right now, but I can still limp along...maybe"  kind
> of
> > > states. :)
> > 
> > If it's something really unexpected like this then NOTREACHED is better,
it's
> > bad enough to receive a corrupted message.
> 
> Is the API contract that the control message is valid before being passed to
> this function?  If so, then I agree with alpha that NOTREACHED is better.
No, it is not API contract here. |message| is just raw message we've received
from the network. We don't know if it is valid or not.

> 
> Then, I would also expect an "IsValidControlMessage" function that we can
> unittest, and which guards the callpoint to this handler.
Yeah, we probably should add more validation code here (e.g. verify that
NotifyResulution contains resolution, etc), but it is not part of this CL.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/fake_sess...
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/fake_sess...
remoting/protocol/fake_session.h:33: int input_pos() { return input_pos_; }
On 2011/01/20 20:06:38, awong wrote:
> const functions here and below?

Done.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_d...
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_d...
remoting/protocol/message_decoder.h:40: // messages yet.
On 2011/01/20 20:06:38, awong wrote:
> Should we note somoewhere that each CompoundBuffer is guaranteed to only have
1
> message?
Extended the comment a bit.

> Should there be a dcheck and/or unittest to verify this assertion
> remains true?
There are unittests for this (message_decoder_unittest,
message_reader_unittest). Also added DCHECK in ProtobufMessageReader.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_d...
File remoting/protocol/message_decoder_unittest.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_d...
remoting/protocol/message_decoder_unittest.cc:89: it != message_list.end();
++it) {
On 2011/01/20 20:06:38, awong wrote:
> Can you add a SCOPED_LOG in this, and the other for loops in this unittest
file
> to make sure we know which iteration we're on (or whatever other information
is
> required to pinpoint the failing input)?

Done.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
File remoting/protocol/message_reader.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.cc:35: void OnDone(CompoundBuffer* buffer) {
It would be great if we could use WeakPtr here, but weak pointers are not thread
safe.

On 2011/01/20 20:06:38, awong wrote:
> 2 thoughts:
>   1) I wonder if we shouldn't delete the buffer back on the jingle_thread. 
> Feels slightly cleaner, and we can collapse OnDone with ProcessDoneEvent.
> 
>   2)  Can't we replace this class with a WeakPtr & funciton?
> 
> Example:
> 
>   static void MessageDone(MessageLoop* run_on_me, WeakPtr<MessageReader>
> message_reader, CompoundBuffer *b) {
>    if (MessageLoop::current() != run_on_me) {
>        // post back to self..
>    }
> 
>    delete b;
>    if (message_reader) {
message_reader may be destroyed on another thread between the line above and the
line below.
>      message_reader->OnMessageDone();
>    }
> }
> 
> ?

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.cc:91: while (!closed_ && !read_pending_ &&
pending_messages_ == 0) {
On 2011/01/20 20:54:52, Alpha wrote:
> The flag of |read_pending| is confusing to me. According to the code we never
> issue two DoRead() at the same time. Also we should never read when there's
> outstanding DoneTaskHandler running, which means when pending_messages > 0,
> read_pending is never true. What I'm saying is that putting !read_pending_ in
> the condition is hiding the problem (which i believe the current code has, see
> my comments below..). Either |read_pending_| should be removed or used as a
> DCHECK to make sure it's exclusive with pending_messages >0.

It is here mainly for the second iteration, after HandleReadResults() sets this
flag. I've added DCHECK.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.cc:131: pending_messages_++;
Good catch! Will also add unittest for this case.

On 2011/01/20 20:54:52, Alpha wrote:
> I think there's a bug here. Incrementing here will not do what you intended
to.
> If the following Run() calls trigger OnMessageDone() then pending_messages
will
> immediately become 0. Either you always post a task in ProcessDoneEvent() or
> change this to become:
> 
> vector<pair<CompoundBuffer*, Task*> > invocations;
> 
> while (true) {
>   CompoundBuffer* buffer = message_decoder_.GetNextMessage();
>   if (!buffer) break;
> }
> 
> pending_messages_ += invocations.size();
> 
> while (invocations...) {
>   message_received_callback_->Run(....);
> }

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
File remoting/protocol/message_reader.h (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.h:25: // callback for each message it receives
On 2011/01/20 20:06:38, awong wrote:
> We should document the flow control semantics here.  In particular, when do we
> enter "read_pending", and how is the client expected to respond?
Added more comments

> 
> I also wonder if we shouldn't just only expose pending_messages, and tell the
> client to throttle based on that rather than whether or not a read itself is
> oustanding...  We might even list outstanding read requests as
> "pending_messages" from the client perspective...
The client already has this information (we call the callback for each message).
Even if we expose pending_messages to the client, it still wouldn't be able to
throttle the stream.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.h:53: bool read_pending_;
On 2011/01/20 20:54:52, Alpha wrote:
> Please add some comments for these variables, it will be very helpful to
> understand how the flow control works.

Done.

[awong, 2011-01-20 22:08:02]

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
File remoting/protocol/client_message_dispatcher.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
remoting/protocol/client_message_dispatcher.cc:49: LOG(ERROR) << "Invalid
control message received.";
On 2011/01/20 21:55:57, sergeyu wrote:
> On 2011/01/20 21:01:56, awong wrote:
> > On 2011/01/20 20:54:52, Alpha wrote:
> > > On 2011/01/20 20:06:38, awong wrote:
> > > > LOG(WARNING) please.
> > > > 
> > > > Let's try to keep LOG(ERROR) for events that are more like "omg! no clue
> > what
> > > is
> > > > happening!  I'd crash right now, but I can still limp along...maybe" 
kind
> > of
> > > > states. :)
> > > 
> > > If it's something really unexpected like this then NOTREACHED is better,
> it's
> > > bad enough to receive a corrupted message.
> > 
> > Is the API contract that the control message is valid before being passed to
> > this function?  If so, then I agree with alpha that NOTREACHED is better.
> No, it is not API contract here. |message| is just raw message we've received
> from the network. We don't know if it is valid or not.
> 
> > 
> > Then, I would also expect an "IsValidControlMessage" function that we can
> > unittest, and which guards the callpoint to this handler.
> Yeah, we probably should add more validation code here (e.g. verify that
> NotifyResulution contains resolution, etc), but it is not part of this CL.

Yep...completley understood.  Can you file a P1 bug for M11, and add a TODO?

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
File remoting/protocol/message_reader.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.cc:35: void OnDone(CompoundBuffer* buffer) {
On 2011/01/20 21:55:57, sergeyu wrote:
> It would be great if we could use WeakPtr here, but weak pointers are not
thread
> safe.

Good point.  Though I think if you do the post back to ensure you don't
reference message_reader_ except from the thread which controls the lifetime of
the underlying object anyways, you can use the weak_ptr here.

Then you could remove both the lock, and the ReleaseReader, no?

> 
> On 2011/01/20 20:06:38, awong wrote:
> > 2 thoughts:
> >   1) I wonder if we shouldn't delete the buffer back on the jingle_thread. 
> > Feels slightly cleaner, and we can collapse OnDone with ProcessDoneEvent.
> > 
> >   2)  Can't we replace this class with a WeakPtr & funciton?
> > 
> > Example:
> > 
> >   static void MessageDone(MessageLoop* run_on_me, WeakPtr<MessageReader>
> > message_reader, CompoundBuffer *b) {
> >    if (MessageLoop::current() != run_on_me) {
> >        // post back to self..
> >    }
> > 
> >    delete b;
> >    if (message_reader) {
> message_reader may be destroyed on another thread between the line above and
the
> line below.

I thought message_reader was created/destroyed on one thread.  The run_on_me
repost should enforce serialization no?

> >      message_reader->OnMessageDone();
> >    }
> > }
> > 
> > ?
> 
>

[Sergey Ulanov, 2011-01-21 03:45:04]

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
File remoting/protocol/client_message_dispatcher.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/client_me...
remoting/protocol/client_message_dispatcher.cc:49: LOG(ERROR) << "Invalid
control message received.";
On 2011/01/20 22:08:02, awong wrote:
> On 2011/01/20 21:55:57, sergeyu wrote:
> > On 2011/01/20 21:01:56, awong wrote:
> > > On 2011/01/20 20:54:52, Alpha wrote:
> > > > On 2011/01/20 20:06:38, awong wrote:
> > > > > LOG(WARNING) please.
> > > > > 
> > > > > Let's try to keep LOG(ERROR) for events that are more like "omg! no
clue
> > > what
> > > > is
> > > > > happening!  I'd crash right now, but I can still limp along...maybe" 
> kind
> > > of
> > > > > states. :)
> > > > 
> > > > If it's something really unexpected like this then NOTREACHED is better,
> > it's
> > > > bad enough to receive a corrupted message.
> > > 
> > > Is the API contract that the control message is valid before being passed
to
> > > this function?  If so, then I agree with alpha that NOTREACHED is better.
> > No, it is not API contract here. |message| is just raw message we've
received
> > from the network. We don't know if it is valid or not.
> > 
> > > 
> > > Then, I would also expect an "IsValidControlMessage" function that we can
> > > unittest, and which guards the callpoint to this handler.
> > Yeah, we probably should add more validation code here (e.g. verify that
> > NotifyResulution contains resolution, etc), but it is not part of this CL.
> 
> Yep...completley understood.  Can you file a P1 bug for M11, and add a TODO?
Added TODO, and filed a bug. I don't agree that this is P1: message stubs should
be protected from invalid messages anyway. Feel free to update 70335 if you
disagree.

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
File remoting/protocol/message_reader.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.cc:35: void OnDone(CompoundBuffer* buffer) {
On 2011/01/20 22:08:02, awong wrote:
> On 2011/01/20 21:55:57, sergeyu wrote:
> > It would be great if we could use WeakPtr here, but weak pointers are not
> thread
> > safe.
> 
> Good point.  Though I think if you do the post back to ensure you don't
> reference message_reader_ except from the thread which controls the lifetime
of
> the underlying object anyways, you can use the weak_ptr here.
> 
> Then you could remove both the lock, and the ReleaseReader, no?
Destructor may be called on a different thread, and my understanding is that
weak_ptr doesn't support that (correct me if I'm wrong here).

> 
> > 
> > On 2011/01/20 20:06:38, awong wrote:
> > > 2 thoughts:
> > >   1) I wonder if we shouldn't delete the buffer back on the jingle_thread.

> > > Feels slightly cleaner, and we can collapse OnDone with ProcessDoneEvent.
> > > 
> > >   2)  Can't we replace this class with a WeakPtr & funciton?
> > > 
> > > Example:
> > > 
> > >   static void MessageDone(MessageLoop* run_on_me, WeakPtr<MessageReader>
> > > message_reader, CompoundBuffer *b) {
> > >    if (MessageLoop::current() != run_on_me) {
> > >        // post back to self..
> > >    }
> > > 
> > >    delete b;
> > >    if (message_reader) {
> > message_reader may be destroyed on another thread between the line above and
> the
> > line below.
> 
> I thought message_reader was created/destroyed on one thread.  The run_on_me
> repost should enforce serialization no?
We don't have any guarantees about the thread on which the object is destroyed.
It is destroyed by scoped_ptr when ConnectionToHost/ConnectionToClient is
destroyed (which usually happens on a thread other than the network), and I
don't think that enforcing a specific thread for destructor is the right
solution.

> 
> > >      message_reader->OnMessageDone();
> > >    }
> > > }
> > > 
> > > ?
> > 
> > 
>

[awong, 2011-01-21 19:19:33]

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
File remoting/protocol/message_reader.cc (right):

http://codereview.chromium.org/6271004/diff/13001/remoting/protocol/message_r...
remoting/protocol/message_reader.cc:35: void OnDone(CompoundBuffer* buffer) {
On 2011/01/21 03:45:05, sergeyu wrote:
> On 2011/01/20 22:08:02, awong wrote:
> > On 2011/01/20 21:55:57, sergeyu wrote:
> > > It would be great if we could use WeakPtr here, but weak pointers are not
> > thread
> > > safe.
> > 
> > Good point.  Though I think if you do the post back to ensure you don't
> > reference message_reader_ except from the thread which controls the lifetime
> of
> > the underlying object anyways, you can use the weak_ptr here.
> > 
> > Then you could remove both the lock, and the ReleaseReader, no?
> Destructor may be called on a different thread, and my understanding is that
> weak_ptr doesn't support that (correct me if I'm wrong here).
> 
> > 
> > > 
> > > On 2011/01/20 20:06:38, awong wrote:
> > > > 2 thoughts:
> > > >   1) I wonder if we shouldn't delete the buffer back on the
jingle_thread.
> 
> > > > Feels slightly cleaner, and we can collapse OnDone with
ProcessDoneEvent.
> > > > 
> > > >   2)  Can't we replace this class with a WeakPtr & funciton?
> > > > 
> > > > Example:
> > > > 
> > > >   static void MessageDone(MessageLoop* run_on_me, WeakPtr<MessageReader>
> > > > message_reader, CompoundBuffer *b) {
> > > >    if (MessageLoop::current() != run_on_me) {
> > > >        // post back to self..
> > > >    }
> > > > 
> > > >    delete b;
> > > >    if (message_reader) {
> > > message_reader may be destroyed on another thread between the line above
and
> > the
> > > line below.
> > 
> > I thought message_reader was created/destroyed on one thread.  The run_on_me
> > repost should enforce serialization no?
> We don't have any guarantees about the thread on which the object is
destroyed.
> It is destroyed by scoped_ptr when ConnectionToHost/ConnectionToClient is
> destroyed (which usually happens on a thread other than the network), and I
> don't think that enforcing a specific thread for destructor is the right
> solution.
> 
> > 
> > > >      message_reader->OnMessageDone();
> > > >    }
> > > > }
> > > > 
> > > > ?
> > > 
> > > 
> > 
> 

Something about this still does not feel right.  In particular, I agree with
alpha that calling OnMessageDone() under a lock is feels error prone.

So the race is that the Connection object can be deleted while a read is
outstanding?  Why can that happen?  Shouldn't we be guarnateeing that all
objects have stopped scheduling tasks before we start the teardown?

[Alpha Left Google, 2011-01-21 23:30:21]

ok, looking much better. LGTM.