Make PacketReader/PacketWriter interfaces async-only.

Make PacketReader/PacketWriter interfaces async-only.

Adapts synchronous I/O completion to use callbacks, so downstream
callers are insulated from having to manage the complexities of
mixed sync and async I/O.

Updated logic of all PacketReader/PacketWriter dependents.

R=haibinlu@chromium.org,wez@chromium.org
BUG=558643
Committed: https://crrev.com/840a0fe2a4cab8709b5d29b89cdb968b620953ff
Cr-Commit-Position: refs/heads/master@{#362833}

[Kevin M, 2015-11-20 21:47:53]

Description was changed from

==========
Make PacketReader/PacketWriter interfaces async-only.

Adapts synchronous I/O completion to use callbacks, so downstream
callers callers are insulated from having to manage the complexities of
mixed sync and async I/O.

R=haibinlu@chromium.org,wez@chromium.org
BUG=558643
==========

to

==========
Make PacketReader/PacketWriter interfaces async-only.

Adapts synchronous I/O completion to use callbacks, so downstream
callers are insulated from having to manage the complexities of
mixed sync and async I/O.

R=haibinlu@chromium.org,wez@chromium.org
BUG=558643
==========

[Wez, 2015-11-20 23:42:13]

https://codereview.chromium.org/1452823011/diff/1/blimp/net/packet_reader.h
File blimp/net/packet_reader.h (right):

https://codereview.chromium.org/1452823011/diff/1/blimp/net/packet_reader.h#n...
blimp/net/packet_reader.h:22: // executed successfully. All other values
indicate errors.
nit: Could we have it invoke w/ net::OK on success and let the caller get the
size directly from |buf|? Or would that mean a resize/copy operation?

https://codereview.chromium.org/1452823011/diff/1/blimp/net/packet_writer.h
File blimp/net/packet_writer.h (right):

https://codereview.chromium.org/1452823011/diff/1/blimp/net/packet_writer.h#n...
blimp/net/packet_writer.h:25: // All other values indicate errors.
nit: "indicate errors" is vague; be explicit as to whether they are recoverable
or not?

Do we want to reserve ERR_IO_PENDING or something to mean "couldn't buffer this
right now, wait til an earlier operation completes"?

https://codereview.chromium.org/1452823011/diff/1/blimp/net/stream_packet_wri...
File blimp/net/stream_packet_writer.cc (right):

https://codereview.chromium.org/1452823011/diff/1/blimp/net/stream_packet_wri...
blimp/net/stream_packet_writer.cc:67: int result = DoWriteLoop(false);
Why are we passing DoWriteLoop a bool here, when it takes an int?

https://codereview.chromium.org/1452823011/diff/1/blimp/net/stream_packet_wri...
blimp/net/stream_packet_writer.cc:70: // synchronously.
Not according to the result test above, it hasn't.

Does this actually pass the unit-tests - if so then we need a new one to trap
this error!

[Kevin M, 2015-11-24 21:34:36]

https://codereview.chromium.org/1452823011/diff/1/blimp/net/packet_reader.h
File blimp/net/packet_reader.h (right):

https://codereview.chromium.org/1452823011/diff/1/blimp/net/packet_reader.h#n...
blimp/net/packet_reader.h:22: // executed successfully. All other values
indicate errors.
On 2015/11/20 23:42:13, Wez wrote:
> nit: Could we have it invoke w/ net::OK on success and let the caller get the
> size directly from |buf|? Or would that mean a resize/copy operation?

Yes, we can advance |offset| to the end.

https://codereview.chromium.org/1452823011/diff/1/blimp/net/packet_writer.h
File blimp/net/packet_writer.h (right):

https://codereview.chromium.org/1452823011/diff/1/blimp/net/packet_writer.h#n...
blimp/net/packet_writer.h:25: // All other values indicate errors.
On 2015/11/20 23:42:13, Wez wrote:
> nit: "indicate errors" is vague; be explicit as to whether they are
recoverable
> or not?
> 
> Do we want to reserve ERR_IO_PENDING or something to mean "couldn't buffer
this
> right now, wait til an earlier operation completes"?

I'm not sure if we want that convention? That seems like it would be useful in
the case where there would be multiple data producers feeding into the same
writer. In our case, it's just the buffer that's pushing data into the writer.

https://codereview.chromium.org/1452823011/diff/1/blimp/net/stream_packet_wri...
File blimp/net/stream_packet_writer.cc (right):

https://codereview.chromium.org/1452823011/diff/1/blimp/net/stream_packet_wri...
blimp/net/stream_packet_writer.cc:67: int result = DoWriteLoop(false);
On 2015/11/20 23:42:13, Wez wrote:
> Why are we passing DoWriteLoop a bool here, when it takes an int?

Done.

https://codereview.chromium.org/1452823011/diff/1/blimp/net/stream_packet_wri...
blimp/net/stream_packet_writer.cc:70: // synchronously.
On 2015/11/20 23:42:13, Wez wrote:
> Not according to the result test above, it hasn't.
> 
> Does this actually pass the unit-tests - if so then we need a new one to trap
> this error!

Done. I must've been interrupted mid-CL, because this portion wasn't done fully.

[Wez, 2015-11-24 23:43:18]

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/packet_reader.h
File blimp/net/packet_reader.h (right):

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/packet_reader...
blimp/net/packet_reader.h:21: // Passes net::OK to |cb| if the read operation 
executed successfully.
nit: Remove double-space

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/packet_reader...
blimp/net/packet_reader.h:22: // The read size can be accessed by calling
|buf.offset()|.
Suggest: Sets |buf.offset()| to the received message's size, and invokes |cb|
with net::OK result, on success.

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
File blimp/net/stream_packet_reader_unittest.cc (right):

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
blimp/net/stream_packet_reader_unittest.cc:48: base::MessageLoop message_loop_;
Is this actually getting used anywhere..?

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
File blimp/net/stream_packet_writer.cc (right):

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
blimp/net/stream_packet_writer.cc:59: return;
If we're mandating no zero-sized packets then writing them indicates something
went badly wrong; I'd be fine with a CHECK(data->BytesRemaining()) so that we
crash rather than send out a zero-byte payload - we'll crash if |data| is NULL,
for example, just later on, in a release build.

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
File blimp/net/stream_packet_writer_unittest.cc (right):

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
blimp/net/stream_packet_writer_unittest.cc:38: ~StreamPacketWriterTest()
override {}
Why override this with a trivial dtor?

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
blimp/net/stream_packet_writer_unittest.cc:41: const std::string test_data_str_
= "U WOT M8";
Is it actually permitted to inline-initialize a non-POD datatype like
std::string?

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
blimp/net/stream_packet_writer_unittest.cc:44: base::MessageLoop message_loop_;
Where is this being used?

[Kevin M, 2015-11-25 01:18:37]

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/packet_reader.h
File blimp/net/packet_reader.h (right):

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/packet_reader...
blimp/net/packet_reader.h:21: // Passes net::OK to |cb| if the read operation 
executed successfully.
On 2015/11/24 23:43:18, Wez wrote:
> nit: Remove double-space

Done.

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/packet_reader...
blimp/net/packet_reader.h:22: // The read size can be accessed by calling
|buf.offset()|.
On 2015/11/24 23:43:18, Wez wrote:
> Suggest: Sets |buf.offset()| to the received message's size, and invokes |cb|
> with net::OK result, on success.

Done.

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
File blimp/net/stream_packet_reader_unittest.cc (right):

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
blimp/net/stream_packet_reader_unittest.cc:48: base::MessageLoop message_loop_;
On 2015/11/24 23:43:18, Wez wrote:
> Is this actually getting used anywhere..?

Yes - the product code bounces sync completion off this MessageLoop. 
TestCompletionCallback::WaitForResult has its own RunLoop that it runs to get
the async result.

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
File blimp/net/stream_packet_writer.cc (right):

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
blimp/net/stream_packet_writer.cc:59: return;
On 2015/11/24 23:43:18, Wez wrote:
> If we're mandating no zero-sized packets then writing them indicates something
> went badly wrong; I'd be fine with a CHECK(data->BytesRemaining()) so that we
> crash rather than send out a zero-byte payload - we'll crash if |data| is
NULL,
> for example, just later on, in a release build.

Done.

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
File blimp/net/stream_packet_writer_unittest.cc (right):

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
blimp/net/stream_packet_writer_unittest.cc:41: const std::string test_data_str_
= "U WOT M8";
On 2015/11/24 23:43:18, Wez wrote:
> Is it actually permitted to inline-initialize a non-POD datatype like
> std::string?

There aren't any prohibitions against non-POD types on the Chromium C++11 page.
In fact, the discussion thread context
(https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0)
has an example w/std::string.

https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
blimp/net/stream_packet_writer_unittest.cc:44: base::MessageLoop message_loop_;
On 2015/11/24 23:43:18, Wez wrote:
> Where is this being used?

Ditto for reader - it's used as the threadlocal MessageLoop for posting sync
completion tasks as async tasks. TestCompletionCallback will run the loop until
idle.

[Wez, 2015-11-25 02:21:28]

lgtm

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/blimp_message...
File blimp/net/blimp_message_pump.cc (right):

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/blimp_message...
blimp/net/blimp_message_pump.cc:70: DCHECK_EQ(result, net::OK);
nit: DCHECK_EQ(net::OK, result)

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/stream_packet...
File blimp/net/stream_packet_writer.cc (right):

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/stream_packet...
blimp/net/stream_packet_writer.cc:54: DCHECK(data->BytesRemaining());
CHECK(data->BytesRemaining()), so that release builds will also barf in this
case?

[Wez, 2015-11-25 02:23:13]

Do we really need this, or could we instead have simple helpers that wrap a
Reader/Writer and do the synchronous->asynchronous PostTask dance - then
the Reader/Writers themselves could still have synchronous optimizations,
while keeping calling code simple by only having to deal with async.

On 24 November 2015 at 17:18, <kmarshall@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1452823011/diff/60001/blimp/net/packet_reader.h
> File blimp/net/packet_reader.h (right):
>
>
>
https://codereview.chromium.org/1452823011/diff/60001/blimp/net/packet_reader...
> blimp/net/packet_reader.h:21: // Passes net::OK to |cb| if the read
> operation  executed successfully.
> On 2015/11/24 23:43:18, Wez wrote:
>
>> nit: Remove double-space
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1452823011/diff/60001/blimp/net/packet_reader...
> blimp/net/packet_reader.h:22: // The read size can be accessed by
> calling |buf.offset()|.
> On 2015/11/24 23:43:18, Wez wrote:
>
>> Suggest: Sets |buf.offset()| to the received message's size, and
>>
> invokes |cb|
>
>> with net::OK result, on success.
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
> File blimp/net/stream_packet_reader_unittest.cc (right):
>
>
>
https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
> blimp/net/stream_packet_reader_unittest.cc:48: base::MessageLoop
> message_loop_;
> On 2015/11/24 23:43:18, Wez wrote:
>
>> Is this actually getting used anywhere..?
>>
>
> Yes - the product code bounces sync completion off this MessageLoop.
> TestCompletionCallback::WaitForResult has its own RunLoop that it runs
> to get the async result.
>
>
>
https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
> File blimp/net/stream_packet_writer.cc (right):
>
>
>
https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
> blimp/net/stream_packet_writer.cc:59: return;
> On 2015/11/24 23:43:18, Wez wrote:
>
>> If we're mandating no zero-sized packets then writing them indicates
>>
> something
>
>> went badly wrong; I'd be fine with a CHECK(data->BytesRemaining()) so
>>
> that we
>
>> crash rather than send out a zero-byte payload - we'll crash if |data|
>>
> is NULL,
>
>> for example, just later on, in a release build.
>>
>
> Done.
>
>
>
https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
> File blimp/net/stream_packet_writer_unittest.cc (right):
>
>
>
https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
> blimp/net/stream_packet_writer_unittest.cc:41: const std::string
> test_data_str_ = "U WOT M8";
> On 2015/11/24 23:43:18, Wez wrote:
>
>> Is it actually permitted to inline-initialize a non-POD datatype like
>> std::string?
>>
>
> There aren't any prohibitions against non-POD types on the Chromium
> C++11 page. In fact, the discussion thread context
> (
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zqB-DySA4V0
> )
> has an example w/std::string.
>
>
>
https://codereview.chromium.org/1452823011/diff/60001/blimp/net/stream_packet...
> blimp/net/stream_packet_writer_unittest.cc:44: base::MessageLoop
> message_loop_;
> On 2015/11/24 23:43:18, Wez wrote:
>
>> Where is this being used?
>>
>
> Ditto for reader - it's used as the threadlocal MessageLoop for posting
> sync completion tasks as async tasks. TestCompletionCallback will run
> the loop until idle.
>
> https://codereview.chromium.org/1452823011/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haibinlu, 2015-11-25 19:10:33]

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/blimp_message...
File blimp/net/blimp_message_pump.cc (right):

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/blimp_message...
blimp/net/blimp_message_pump.cc:48: // The result is the size of the packet in
bytes.
so, ReadPacket no longer returns the size of the packet in bytes on success? 

update PacketReader.ReadPacket description.

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/blimp_message...
blimp/net/blimp_message_pump.cc:63: DCHECK_NE(net::OK, result);
is this obviously true?

[Kevin M, 2015-11-30 19:14:48]

IMO, having interfaces that expose mixed sync/async seems to be more of a burden
than an asset if we don't have support for it plumbed across MessageProcessors.

My preference would be to keep the interfaces simple and async-only for now. We
can revisit this later to see if the hit to performance is non-negligible.

[Kevin M, 2015-11-30 19:25:14]

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/blimp_message...
File blimp/net/blimp_message_pump.cc (right):

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/blimp_message...
blimp/net/blimp_message_pump.cc:48: // The result is the size of the packet in
bytes.
On 2015/11/25 19:10:33, haibinlu wrote:
> so, ReadPacket no longer returns the size of the packet in bytes on success? 
> 
> update PacketReader.ReadPacket description.

The interface was changed in response to feedback for this CL. We will use the
buffer's offset to communicate the size to callers.

PacketReader.ReadPacket() comment is already up-to-date.

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/blimp_message...
blimp/net/blimp_message_pump.cc:63: DCHECK_NE(net::OK, result);
On 2015/11/25 19:10:33, haibinlu wrote:
> is this obviously true?

Yes - the only success case returns out of the function.

I added an "else" before setting net::ERR_FAILED to make this more explicit.

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/blimp_message...
blimp/net/blimp_message_pump.cc:70: DCHECK_EQ(result, net::OK);
On 2015/11/25 02:21:28, Wez wrote:
> nit: DCHECK_EQ(net::OK, result)

Done.

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/stream_packet...
File blimp/net/stream_packet_writer.cc (right):

https://codereview.chromium.org/1452823011/diff/80001/blimp/net/stream_packet...
blimp/net/stream_packet_writer.cc:54: DCHECK(data->BytesRemaining());
On 2015/11/25 02:21:28, Wez wrote:
> CHECK(data->BytesRemaining()), so that release builds will also barf in this
> case?

Done.

[Wez, 2015-11-30 21:33:24]

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection.cc (right):

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:75: write_packet_callback_.Reset(base::Bind(
You only need write_packet_callback_ to ensure that any in-flight
write-completion is cancelled if this sender object is torn down - you could
make it part of the PacketWriter API contract that implementations must cancel
pending completion callbacks if they're deleted, so that callers don't need to
worry about that.

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/blimp_messag...
File blimp/net/blimp_message_pump.cc (right):

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.cc:43: reader_->ReadPacket(buffer_.get(),
read_packet_callback_.callback());
See my other comment re cancellable callbacks.

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.cc:55: } else {
Never use constructs of the form if (condition) { ... return; } else { } - the
else ends up being redundant since if |condition| is true it can never be
reached, so it just obfuscates the code a little. :P

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/packet_reader.h
File blimp/net/packet_reader.h (right):

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/packet_reade...
blimp/net/packet_reader.h:24: // All other values indicate errors.
Need to document the semantics if a read operation is pending when PacketReader
is destroyed - will the callback be notified, or not, or maybe not?

Whichever semantics we choose, including unit-tests that verify either that the
callers, or the reader/writer impls, obey the constraints would be a good idea.

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/packet_writer.h
File blimp/net/packet_writer.h (right):

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/packet_write...
blimp/net/packet_writer.h:25: // All other values indicate unrecoverable errors.
As for the reader, define the semantics wrt |cv| being called, or not being
called, after the writer has been deleted.

[haibinlu, 2015-11-30 21:50:31]

lgtm

[Kevin M, 2015-12-01 01:50:27]

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection.cc (right):

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:75: write_packet_callback_.Reset(base::Bind(
On 2015/11/30 21:33:24, Wez wrote:
> You only need write_packet_callback_ to ensure that any in-flight
> write-completion is cancelled if this sender object is torn down - you could
> make it part of the PacketWriter API contract that implementations must cancel
> pending completion callbacks if they're deleted, so that callers don't need to
> worry about that.

Done.

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/blimp_messag...
File blimp/net/blimp_message_pump.cc (right):

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.cc:43: reader_->ReadPacket(buffer_.get(),
read_packet_callback_.callback());
On 2015/11/30 21:33:24, Wez wrote:
> See my other comment re cancellable callbacks.

Done.

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.cc:55: } else {
On 2015/11/30 21:33:24, Wez wrote:
> Never use constructs of the form if (condition) { ... return; } else { } - the
> else ends up being redundant since if |condition| is true it can never be
> reached, so it just obfuscates the code a little. :P

Done.

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/packet_reader.h
File blimp/net/packet_reader.h (right):

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/packet_reade...
blimp/net/packet_reader.h:24: // All other values indicate errors.
On 2015/11/30 21:33:24, Wez wrote:
> Need to document the semantics if a read operation is pending when
PacketReader
> is destroyed - will the callback be notified, or not, or maybe not?
> 
> Whichever semantics we choose, including unit-tests that verify either that
the
> callers, or the reader/writer impls, obey the constraints would be a good
idea.

Done.

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/packet_writer.h
File blimp/net/packet_writer.h (right):

https://codereview.chromium.org/1452823011/diff/100001/blimp/net/packet_write...
blimp/net/packet_writer.h:25: // All other values indicate unrecoverable errors.
On 2015/11/30 21:33:24, Wez wrote:
> As for the reader, define the semantics wrt |cv| being called, or not being
> called, after the writer has been deleted.

Done.

[Kevin M, 2015-12-01 19:28:17]

Description was changed from

==========
Make PacketReader/PacketWriter interfaces async-only.

Adapts synchronous I/O completion to use callbacks, so downstream
callers are insulated from having to manage the complexities of
mixed sync and async I/O.

R=haibinlu@chromium.org,wez@chromium.org
BUG=558643
==========

to

==========
Make PacketReader/PacketWriter interfaces async-only.

Adapts synchronous I/O completion to use callbacks, so downstream
callers are insulated from having to manage the complexities of
mixed sync and async I/O.

Updated logic of all PacketReader/PacketWriter dependents.

R=haibinlu@chromium.org,wez@chromium.org
BUG=558643
==========

[Kevin M, 2015-12-01 19:30:09]

Haibin/Wez, I had to make some additional changes to the test code, some of
which involved removing superfluous tests and revisiting some assumptions (e.g.
making BlimpMessagePump single-use instead of reusable.)

[Wez, 2015-12-01 22:34:45]

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/blimp_messag...
File blimp/net/blimp_message_pump.h (right):

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:56: net::CancelableCompletionCallback
process_msg_callback_;
nit: It would be helpful to have a one-line comment to explain why this needs to
be cancelable.

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:57: net::CancelableCompletionCallback
read_callback_;
Again, it'd be good to explain why we need a cancelable callback here - looks
like it's just-in-case the caller deletes the pump but leaves the PacketReader
active while there's a read still pending.

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/packet_reader.h
File blimp/net/packet_reader.h (right):

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/packet_reade...
blimp/net/packet_reader.h:25: // |callback| must not be invoked if |this| is
deleted.
nit: must -> will

That way the comment describes the semantics from the PoV of what callers can
expect.

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/stream_packe...
File blimp/net/stream_packet_reader.cc (right):

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/stream_packe...
blimp/net/stream_packet_reader.cc:65: base::Bind(callback, result));
What happens if |this| is deleted by the caller before this task is run?

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/stream_packe...
File blimp/net/stream_packet_writer.cc (right):

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/stream_packe...
blimp/net/stream_packet_writer.cc:70: base::Bind(callback, result));
What happens if _this_ is deleted by the caller before this task is run?

[Kevin M, 2015-12-02 02:02:37]

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/blimp_messag...
File blimp/net/blimp_message_pump.h (right):

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:56: net::CancelableCompletionCallback
process_msg_callback_;
On 2015/12/01 22:34:45, Wez wrote:
> nit: It would be helpful to have a one-line comment to explain why this needs
to
> be cancelable.

Done.

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:57: net::CancelableCompletionCallback
read_callback_;
On 2015/12/01 22:34:45, Wez wrote:
> Again, it'd be good to explain why we need a cancelable callback here - looks
> like it's just-in-case the caller deletes the pump but leaves the PacketReader
> active while there's a read still pending.

Done.

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/packet_reader.h
File blimp/net/packet_reader.h (right):

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/packet_reade...
blimp/net/packet_reader.h:25: // |callback| must not be invoked if |this| is
deleted.
On 2015/12/01 22:34:45, Wez wrote:
> nit: must -> will
> 
> That way the comment describes the semantics from the PoV of what callers can
> expect.

Done.

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/stream_packe...
File blimp/net/stream_packet_reader.cc (right):

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/stream_packe...
blimp/net/stream_packet_reader.cc:65: base::Bind(callback, result));
On 2015/12/01 22:34:45, Wez wrote:
> What happens if |this| is deleted by the caller before this task is run?

Seems fine to me - |callback| is supplied by the caller and doesn't refer to
anything in StreamPacketReader?

In practice, the caller is BlimpMessagePump, which will be destroyed before
StreamPacketReader and uses a CancelableCallback for its read cb.

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/stream_packe...
File blimp/net/stream_packet_writer.cc (right):

https://codereview.chromium.org/1452823011/diff/140001/blimp/net/stream_packe...
blimp/net/stream_packet_writer.cc:70: base::Bind(callback, result));
On 2015/12/01 22:34:45, Wez wrote:
> What happens if _this_ is deleted by the caller before this task is run?

This seems fine? |callback| doesn't re-enter any code here. The outgoing message
processor will be removed from the output buffer when |this| is deleted, so the
output buffer is prevented from accessing |this| in response to a callback.

[Kevin M, 2015-12-02 19:24:40]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2015-12-02 19:24:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
haibinlu@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1452823011/#ps160001
(title: "Wez feedback")

[commit-bot: I haz the power, 2015-12-02 19:25:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1452823011/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1452823011/160001

[commit-bot: I haz the power, 2015-12-02 19:31:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-02 19:31:43]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kevin M, 2015-12-02 23:24:25]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2015-12-02 23:24:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
haibinlu@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1452823011/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-02 23:26:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1452823011/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1452823011/180001

[commit-bot: I haz the power, 2015-12-03 00:30:06]

Description was changed from

==========
Make PacketReader/PacketWriter interfaces async-only.

Adapts synchronous I/O completion to use callbacks, so downstream
callers are insulated from having to manage the complexities of
mixed sync and async I/O.

Updated logic of all PacketReader/PacketWriter dependents.

R=haibinlu@chromium.org,wez@chromium.org
BUG=558643
==========

to

==========
Make PacketReader/PacketWriter interfaces async-only.

Adapts synchronous I/O completion to use callbacks, so downstream
callers are insulated from having to manage the complexities of
mixed sync and async I/O.

Updated logic of all PacketReader/PacketWriter dependents.

R=haibinlu@chromium.org,wez@chromium.org
BUG=558643
==========

[commit-bot: I haz the power, 2015-12-03 00:30:07]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-12-03 00:31:04]

Description was changed from

==========
Make PacketReader/PacketWriter interfaces async-only.

Adapts synchronous I/O completion to use callbacks, so downstream
callers are insulated from having to manage the complexities of
mixed sync and async I/O.

Updated logic of all PacketReader/PacketWriter dependents.

R=haibinlu@chromium.org,wez@chromium.org
BUG=558643
==========

to

==========
Make PacketReader/PacketWriter interfaces async-only.

Adapts synchronous I/O completion to use callbacks, so downstream
callers are insulated from having to manage the complexities of
mixed sync and async I/O.

Updated logic of all PacketReader/PacketWriter dependents.

R=haibinlu@chromium.org,wez@chromium.org
BUG=558643

Committed: https://crrev.com/840a0fe2a4cab8709b5d29b89cdb968b620953ff
Cr-Commit-Position: refs/heads/master@{#362833}
==========

[commit-bot: I haz the power, 2015-12-03 00:31:05]

Patchset 10 (id:??) landed as
https://crrev.com/840a0fe2a4cab8709b5d29b89cdb968b620953ff
Cr-Commit-Position: refs/heads/master@{#362833}