[mojo] Implement data pipe using a shared buffer.

[mojo] Implement data pipe using a shared buffer.

The general modification is as follows: Store a ring buffer using a shared buffer. Each side  maintains its own state of the ring buffer, and tells the other side when it reads or writes. Since one side might not be able to create a shared buffer (due to permissions), one side will try to create it, and if it can't, ask the other side to create it.

BUG=556258

[Eliot Courtney, 2015-12-21 23:22:56]

edcourtney@chromium.org changed reviewers:
+ amistry@chromium.org, jam@chromium.org, sammc@chromium.org

[Eliot Courtney, 2015-12-21 23:22:57]

Hi, this is my attempt at adding shared buffers to data pipe. Sorry this is such
a big CL, I couldn't figure out how to split it up without increasing the total
complexity a lot. If you have a way for me to split this CL, I can do that.

The general modification is as follows: Store a ring buffer using a shared
buffer. Each side  maintains its own state of the ring buffer, and tells the
other side when it reads or writes. Since one side might not be able to create a
shared buffer (due to permissions), one side will try to create it, and if it
can't, ask the other side to create it.

This modification makes the producer and consumer more symmetric so I've moved a
lot of code (particulary wrt ring buffer handling and serialisation) into the
common DataPipe class, which I've made a member of the producer and consumer.

I'm very new to Mojo so I'm sorry if there are any really silly mistakes.

[Eliot Courtney, 2015-12-21 23:33:56]

Description was changed from

==========
[DO NOT SUBMIT] test datapipe impl
==========

to

==========
[mojo] Implement data pipe using a shared buffer.

The general modification is as follows: Store a ring buffer using a shared
buffer. Each side  maintains its own state of the ring buffer, and tells the
other side when it reads or writes. Since one side might not be able to create a
shared buffer (due to permissions), one side will try to create it, and if it
can't, ask the other side to create it.

BUG=556258
==========

[jam, 2015-12-28 17:11:02]

Thanks for working on this Eliot!

I haven't looked at the code in detail yet, I did have a preliminary comment
based on this description.

On 2015/12/21 23:22:57, edcourtney wrote:
> Hi, this is my attempt at adding shared buffers to data pipe. Sorry this is
such
> a big CL, I couldn't figure out how to split it up without increasing the
total
> complexity a lot. If you have a way for me to split this CL, I can do that.
> 
> The general modification is as follows: Store a ring buffer using a shared
> buffer. Each side  maintains its own state of the ring buffer, and tells the
> other side when it reads or writes. Since one side might not be able to create
a
> shared buffer (due to permissions), one side will try to create it, and if it
> can't, ask the other side to create it.

This works if the data pipe is always between the unsandboxed browser process
and a sandboxed child process. However data pipes can be between any two
arbitrary processes, which includes two sandboxed child processes. In that case,
both processes might not be able to create a shared memory buffer on POSIX (on
Windows this is fine, i.e. see the non mojo code
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/chil...).

This means we'll need a process to have a privileged broker help it to create
shared memory. We've already had to solve this problem on Windows, for creating
platform channel pairs (i.e. win32's CreateNamedPipe) and also passing platform
handles between processes (i.e. win32's DuplicateHandle). This is in
mojo/edk/system/*broker*. You can start by looking at broker.h which is the
interface that's used by Core.

If the creation of the shared memory can be asynchronous, then that's easier
because the async part of the broker interface works on posix and windows. If it
needs to be synchronous, then we'll have to make the synchronous parts of that
interface work on posix as well. Currently I only made it work on Windows
because that's the only platform that needed it so far. This shouldn't be much
work; mostly making the OS_WIN sections in child_broker_host.cc and
child_broker.cc be cross platform. this is a much simpler interface than what
RawChannel provides, and we already have utility methods to handle the
read/writing part in platform_channel_utils_posix.h

If anything I said isn't unclear, happy to chat in VC

> 
> This modification makes the producer and consumer more symmetric so I've moved
a
> lot of code (particulary wrt ring buffer handling and serialisation) into the
> common DataPipe class, which I've made a member of the producer and consumer.
> 
> I'm very new to Mojo so I'm sorry if there are any really silly mistakes.

[Eliot Courtney, 2015-12-30 23:35:40]

> This works if the data pipe is always between the unsandboxed browser process
> and a sandboxed child process. However data pipes can be between any two
> arbitrary processes, which includes two sandboxed child processes. In that
case,
> both processes might not be able to create a shared memory buffer on POSIX (on
> Windows this is fine, i.e. see the non mojo code
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/chil...).
> 
> This means we'll need a process to have a privileged broker help it to create
> shared memory. We've already had to solve this problem on Windows, for
creating
> platform channel pairs (i.e. win32's CreateNamedPipe) and also passing
platform
> handles between processes (i.e. win32's DuplicateHandle). This is in
> mojo/edk/system/*broker*. You can start by looking at broker.h which is the
> interface that's used by Core.
> 
> If the creation of the shared memory can be asynchronous, then that's easier
> because the async part of the broker interface works on posix and windows. If
it
> needs to be synchronous, then we'll have to make the synchronous parts of that
> interface work on posix as well. Currently I only made it work on Windows
> because that's the only platform that needed it so far. This shouldn't be much
> work; mostly making the OS_WIN sections in child_broker_host.cc and
> child_broker.cc be cross platform. this is a much simpler interface than what
> RawChannel provides, and we already have utility methods to handle the
> read/writing part in platform_channel_utils_posix.h
> 
> If anything I said isn't unclear, happy to chat in VC

Hi, thanks for the advice! I had a look at it, and since shared buffers are used
in two places:
1. the creation of the data pipe, and 2. serialisation for storing RawChannel's
read/write
buffers (after calling Release()), it seemed a little hard to use an
asynchronous interface to me.
I could change the serialisation to store RawChannel's read/write data in the
message itself,
rather than a shared buffer. But, it would mean that serialisation messages
would not be a
constant size and so potentially it might require multiple messages to send the
serialised state, which seemed messy to me.

For now, I've started on adding a synchronous 'CreateSharedBuffer(num_bytes)'
method to Broker, but
I'm still trying to iron out some bugs with it (so it's not uploaded yet). I was
also wondering
if you think it would make sense to put that in a separate cl?

Thanks.

[jam, 2016-01-04 15:39:14]

On 2015/12/30 23:35:40, Eliot Courtney wrote:
> > This works if the data pipe is always between the unsandboxed browser
process
> > and a sandboxed child process. However data pipes can be between any two
> > arbitrary processes, which includes two sandboxed child processes. In that
> case,
> > both processes might not be able to create a shared memory buffer on POSIX
(on
> > Windows this is fine, i.e. see the non mojo code
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/chil...).
> > 
> > This means we'll need a process to have a privileged broker help it to
create
> > shared memory. We've already had to solve this problem on Windows, for
> creating
> > platform channel pairs (i.e. win32's CreateNamedPipe) and also passing
> platform
> > handles between processes (i.e. win32's DuplicateHandle). This is in
> > mojo/edk/system/*broker*. You can start by looking at broker.h which is the
> > interface that's used by Core.
> > 
> > If the creation of the shared memory can be asynchronous, then that's easier
> > because the async part of the broker interface works on posix and windows.
If
> it
> > needs to be synchronous, then we'll have to make the synchronous parts of
that
> > interface work on posix as well. Currently I only made it work on Windows
> > because that's the only platform that needed it so far. This shouldn't be
much
> > work; mostly making the OS_WIN sections in child_broker_host.cc and
> > child_broker.cc be cross platform. this is a much simpler interface than
what
> > RawChannel provides, and we already have utility methods to handle the
> > read/writing part in platform_channel_utils_posix.h
> > 
> > If anything I said isn't unclear, happy to chat in VC
> 
> Hi, thanks for the advice! I had a look at it, and since shared buffers are
used
> in two places:
> 1. the creation of the data pipe, and 2. serialisation for storing
RawChannel's
> read/write
> buffers (after calling Release()), it seemed a little hard to use an
> asynchronous interface to me.
> I could change the serialisation to store RawChannel's read/write data in the
> message itself,
> rather than a shared buffer. But, it would mean that serialisation messages
> would not be a
> constant size and so potentially it might require multiple messages to send
the
> serialised state, which seemed messy to me.

ah yes, you're right. synchronous it is :)
> 
> For now, I've started on adding a synchronous 'CreateSharedBuffer(num_bytes)'
> method to Broker, but
> I'm still trying to iron out some bugs with it (so it's not uploaded yet). I
was
> also wondering
> if you think it would make sense to put that in a separate cl?

if it's small, ok to include it here. if it's large, might be good to split it
out just to make reviewing easier.

> 
> Thanks.

[Anand Mistry (off Chromium), 2016-01-08 03:28:02]

I'm still reading this, but here are some initial comments.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe.cc (right):

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.cc:359: size_t bufsz = options_.capacity_num_bytes;
This isn't Go. Don't abbreviate unnecessarily.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.cc:361: if (base == nullptr) {
generally prefer:
if (!base)

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe.h (right):

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:26: enum DataPipeCommand {
: uint32_t
That way, you can just use this type in the struct below.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:27: PLEASE_CREATE_SHARED_BUFFER,
With the broker, you no longer need to do this. It should help simplify the code
a fair bit.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:35: size_t num_bytes;
Never use size_t in a serialised message.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:149: void* GetSharedBufferBase();
Make this a uint8_t*. That way, you can avoid all the casts.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe_consumer_dispatcher.cc:38: ring_buffer_start,
ring_buffer_size, false,
ring_buffer_start, ring_buffer_size, false /* is_producer */,

Just having 'false' is non-descriptive.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe_unittest.cc (right):

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe_unittest.cc:381:
ASSERT_EQ(MOJO_HANDLE_SIGNAL_WRITABLE, hss.satisfied_signals);
EXPECT_EQ and next one.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe_unittest.cc:414:
ASSERT_EQ(MOJO_HANDLE_SIGNAL_WRITABLE, hss.satisfied_signals);
EXPECT_EQ and next one.

[Eliot Courtney, 2016-01-08 04:44:40]

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe.cc (right):

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.cc:359: size_t bufsz = options_.capacity_num_bytes;
On 2016/01/08 03:28:02, Anand Mistry wrote:
> This isn't Go. Don't abbreviate unnecessarily.

Done.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.cc:361: if (base == nullptr) {
On 2016/01/08 03:28:02, Anand Mistry wrote:
> generally prefer:
> if (!base)

Done.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe.h (right):

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:26: enum DataPipeCommand {
On 2016/01/08 03:28:02, Anand Mistry wrote:
> : uint32_t
> That way, you can just use this type in the struct below.

Done.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:27: PLEASE_CREATE_SHARED_BUFFER,
On 2016/01/08 03:28:02, Anand Mistry wrote:
> With the broker, you no longer need to do this. It should help simplify the
code
> a fair bit.

Whoops, I already did this but forgot to upload it. Thanks!

Done.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:35: size_t num_bytes;
On 2016/01/08 03:28:02, Anand Mistry wrote:
> Never use size_t in a serialised message.

Done.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:149: void* GetSharedBufferBase();
On 2016/01/08 03:28:02, Anand Mistry wrote:
> Make this a uint8_t*. That way, you can avoid all the casts.

Done.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe_consumer_dispatcher.cc:38: ring_buffer_start,
ring_buffer_size, false,
On 2016/01/08 03:28:02, Anand Mistry wrote:
> ring_buffer_start, ring_buffer_size, false /* is_producer */,
> 
> Just having 'false' is non-descriptive.

Done.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe_unittest.cc (right):

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe_unittest.cc:381:
ASSERT_EQ(MOJO_HANDLE_SIGNAL_WRITABLE, hss.satisfied_signals);
On 2016/01/08 03:28:02, Anand Mistry wrote:
> EXPECT_EQ and next one.

Done.

https://codereview.chromium.org/1526923006/diff/160001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe_unittest.cc:414:
ASSERT_EQ(MOJO_HANDLE_SIGNAL_WRITABLE, hss.satisfied_signals);
On 2016/01/08 03:28:02, Anand Mistry wrote:
> EXPECT_EQ and next one.

Done.

[Anand Mistry (off Chromium), 2016-01-11 06:19:35]

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe.cc (right):

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.cc:124: void DataPipe::Init(ScopedPlatformHandle
message_pipe,
s/message_pipe/channel
where appropriate.

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.cc:193: serialized_channel_handle_ =
channel_->ReleaseHandle(
From what I can tell, this is racy. Basically, this ends up deleting the
read/write MessageLoopForIO::FileDescriptorWatcher's (on posix) from a non-IO
thread. Based on my reading of libevent/message_pump_libevent, this is incorrect
and racy behaviour.

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.cc:586: DCHECK(0);
NOTREACHED();

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe.h (right):

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:27: NOTIFY_SHARED_BUFFER,
I'm curious why you need this functionality. Since both sides are created at the
same time in the same process, why not just create the shared buffer and pass it
into both instances of DataPipe?

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:60: void Init(ScopedPlatformHandle message_pipe,
Most of these arguments are details about the serialisation. If deserialisation
was mostly pushed into DataPipe, you wouldn't need most of these.

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:148: // True if we've released the channel because
we're about to get serialised
Unfortunately, we use american spelling. So 'serialize'.

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe_consumer_dispatcher.cc:66: ScopedPlatformHandle
channel_handle, channel_shared_handle,
It feels like most of this code should be encapsulated inside DataPipe. Really,
DataPipe*Dispatcher should be mostly a light-ish wrapper around DataPipe, which
should be doing most of the heavy lifting (including
serialisation/deserialisation).

[Eliot Courtney, 2016-01-13 00:00:10]

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe.cc (right):

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.cc:124: void DataPipe::Init(ScopedPlatformHandle
message_pipe,
On 2016/01/11 06:19:34, Anand Mistry wrote:
> s/message_pipe/channel
> where appropriate.

Done.

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.cc:586: DCHECK(0);
On 2016/01/11 06:19:34, Anand Mistry wrote:
> NOTREACHED();

Done.

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe.h (right):

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:27: NOTIFY_SHARED_BUFFER,
On 2016/01/11 06:19:34, Anand Mistry wrote:
> I'm curious why you need this functionality. Since both sides are created at
the
> same time in the same process, why not just create the shared buffer and pass
it
> into both instances of DataPipe?

Done.

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:60: void Init(ScopedPlatformHandle message_pipe,
On 2016/01/11 06:19:34, Anand Mistry wrote:
> Most of these arguments are details about the serialisation. If
deserialisation
> was mostly pushed into DataPipe, you wouldn't need most of these.

Done.

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:148: // True if we've released the channel because
we're about to get serialised
On 2016/01/11 06:19:34, Anand Mistry wrote:
> Unfortunately, we use american spelling. So 'serialize'.

Done.

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe_consumer_dispatcher.cc:66: ScopedPlatformHandle
channel_handle, channel_shared_handle,
On 2016/01/11 06:19:34, Anand Mistry wrote:
> It feels like most of this code should be encapsulated inside DataPipe.
Really,
> DataPipe*Dispatcher should be mostly a light-ish wrapper around DataPipe,
which
> should be doing most of the heavy lifting (including
> serialisation/deserialisation).

Done.

[Eliot Courtney, 2016-01-14 02:17:57]

jam: ping -- thanks!

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe.cc (right):

https://codereview.chromium.org/1526923006/diff/240001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.cc:193: serialized_channel_handle_ =
channel_->ReleaseHandle(
On 2016/01/11 06:19:34, Anand Mistry wrote:
> From what I can tell, this is racy. Basically, this ends up deleting the
> read/write MessageLoopForIO::FileDescriptorWatcher's (on posix) from a non-IO
> thread. Based on my reading of libevent/message_pump_libevent, this is
incorrect
> and racy behaviour.

Discussed offline -- this is fairly impossible to fix with the EDK design, and
also no more racy than what previously existed.

[jam, 2016-01-15 02:16:44]

Sorry just saw the ping (feel free to IM me if I don't respond quickly).

A question about moving code to the shared class is my main comment. It's hard
to check the ring buffer code for correctness by inspection. Unfortunately until
https://codereview.chromium.org/1554443003/ lands, you have to run
mojo_system_unittests2 manually (note the "2").

Have you also tried running mandoline's page cycler? That should be a good
stress test to ensure correctness:
https://www.chromium.org/developers/mandoline/build

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe.h (right):

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:40: class MOJO_SYSTEM_IMPL_EXPORT DataPipe final
it's a bit confusing that this shared class has method for reading and writing
but only one set can be called in an instance.

why not separate the reading/writing stuff into the respective class
(DataPipeConsumerDispatcher and DPDD) and only have the shared code here?

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:100: RawChannel* GetChannel() { return channel_; }
nit: channel()

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:101: void SetChannel(RawChannel* channel);
nit: set_channel() inline (dcheck is fine inline)

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:102: void
SetSharedBuffer(scoped_refptr<PlatformSharedBuffer> shared_buffer) {
nit: set_shared_buffer()

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:105: const MojoCreateDataPipeOptions& GetOptions() {
return options_; }
nit: options() (this is all from the chrome/google style guide)

[Eliot Courtney, 2016-01-15 05:10:34]

mojo_system_unittests2 passes; I also ran page_cycler.typical_25 using the site
you linked me (thanks!) and it worked, although it didn't seem to use DataPipe
-- maybe I ran the wrong thing though?

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe.h (right):

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:40: class MOJO_SYSTEM_IMPL_EXPORT DataPipe final
On 2016/01/15 02:16:44, jam wrote:
> it's a bit confusing that this shared class has method for reading and writing
> but only one set can be called in an instance.
> 
> why not separate the reading/writing stuff into the respective class
> (DataPipeConsumerDispatcher and DPDD) and only have the shared code here?

I agree that it's strange that you can only use one set of them. I think moving
code out into dispatcher that reaches into DataPipe's internals and modifies
them would be harder to understand. I could add a boolean saying whether or not
DataPipe is acting as a consumer and dispatcher and DCHECK or explicitly
disallow calls, which might make it easier to see what's going on. WDYT?

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:100: RawChannel* GetChannel() { return channel_; }
On 2016/01/15 02:16:44, jam wrote:
> nit: channel()

Done.

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:101: void SetChannel(RawChannel* channel);
On 2016/01/15 02:16:44, jam wrote:
> nit: set_channel() inline (dcheck is fine inline)

Done.

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:102: void
SetSharedBuffer(scoped_refptr<PlatformSharedBuffer> shared_buffer) {
On 2016/01/15 02:16:44, jam wrote:
> nit: set_shared_buffer()

Done.

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:105: const MojoCreateDataPipeOptions& GetOptions() {
return options_; }
On 2016/01/15 02:16:44, jam wrote:
> nit: options() (this is all from the chrome/google style guide)

Sorry, thanks!

[jam, 2016-01-15 16:24:07]

On 2016/01/15 05:10:34, Eliot Courtney wrote:
> mojo_system_unittests2 passes; I also ran page_cycler.typical_25 using the
site
> you linked me (thanks!) and it worked, although it didn't seem to use DataPipe
> -- maybe I ran the wrong thing though?

You ran mandoline right? You're sure it didn't go through data pipe? Because
Mandoline does use data pipe for network service.

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe.h (right):

https://codereview.chromium.org/1526923006/diff/260001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe.h:40: class MOJO_SYSTEM_IMPL_EXPORT DataPipe final
On 2016/01/15 05:10:34, Eliot Courtney wrote:
> On 2016/01/15 02:16:44, jam wrote:
> > it's a bit confusing that this shared class has method for reading and
writing
> > but only one set can be called in an instance.
> > 
> > why not separate the reading/writing stuff into the respective class
> > (DataPipeConsumerDispatcher and DPDD) and only have the shared code here?
> 
> I agree that it's strange that you can only use one set of them. I think
moving
> code out into dispatcher that reaches into DataPipe's internals and modifies
> them would be harder to understand.

It's hard, conceptually without seeing code, to see why that would be the case.
Can you upload a version of that to compare?

It seems like we can come up with a small number of methods to modify state, and
have appropriate getters for whatever the owning classes need.

> I could add a boolean saying whether or not
> DataPipe is acting as a consumer and dispatcher and DCHECK or explicitly
> disallow calls, which might make it easier to see what's going on. WDYT?

this doesn't address the issue though that there are two distinct pathways in
this class that are quite different.