Add support for sockets that can listen and accept a connection.

Add support for sockets that can listen and accept a connection.

These sockets allow one connection at a time, however clients can
connect and disconnect repeatedly.

These are going to be used by Cloud Print, Remoting and
Automation.

BUG=NONE
TEST=BUILD

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

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

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

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

[dmac, 2010-12-09 21:46:28]

Any comments appreciated ;-)

[dmac, 2010-12-09 21:47:30]

BTW Thakis, I included the switches CL in this one intentionally. Jeremy ok'd
the other one, but I made a silly mistake on my side and it was easier to
include it here. Sorry for any confusion.

On 2010/12/09 21:46:28, dmac wrote:
> Any comments appreciated ;-)

[Nico, 2010-12-10 18:08:03]

Hi Dave, I'm not too familiar with sockets, so unless Evan chimes in, this will
be slow.

(Wooo, I'm in the same Reviewers: line as Evan!)

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:345: Mode mode) {
do you need the |mode| parameter?

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:358: bool uses_domain_socket = false;
= mode_ == MODE_NAMED_SERVER || mode_ == MODE_NAMED_CLIENT?

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:361: mode_ = MODE_SERVER;
|mode_| changing somewhere in the middle of this class feels a bit magical.
Maybe you can keep this in the constructor and pass |uses_domain_socket| in as
parameter?

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.h
File ipc/ipc_channel_posix.h (right):

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.h#newc...
ipc/ipc_channel_posix.h:138: // true if we responsible for unlinking the unix
domain socket file.
no verb

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
File ipc/ipc_channel_posix_unittest.cc (right):

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
ipc/ipc_channel_posix_unittest.cc:137: // in the case of a bad test.
"Usually, the run loop will quit sooner than that because all tests use a
IPCChannelPosixTestListener which quits the current run loop on any channel
activity."?

Is there some way you can enforce that this method isn't called without this
listener in place?

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
ipc/ipc_channel_posix_unittest.cc:158: ASSERT_EQ(socketpair(AF_UNIX,
SOCK_STREAM, 0, pipe_fds), 0);
the argument order for ASSERT is expected, actual

[Nico, 2010-12-10 18:48:07]

To my uninformed eyes, this looks ok. Maybe you want to ask jeremy and/or agl to
take a look, it seems like they wrote huge chunks of this file.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:458: listening = count > 0 ? 0 : 1;
drop | ? 0 : 1|

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:1116: unlink(pipe_name_.c_str());
Do you need to set must_link_ to false here? See comment about idempotency
above.

[Paweł Hajdan Jr., 2010-12-10 18:48:15]

Drive-by with test comments. Could you let me take another look before
committing?

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
File ipc/ipc_channel_posix_unittest.cc (right):

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
ipc/ipc_channel_posix_unittest.cc:22: static const int kIPCPosixTestTimeOutInMS
= 2000;
Please use base/test/test_timeouts.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
ipc/ipc_channel_posix_unittest.cc:288: while (true) {
Why do we spin the loop multiple times? Can't we do it just once?

[jeremy, 2010-12-12 14:59:29]

A few driveby comments.

Could you check that this patch doesn't impact performance?  I'm sure Pawel,
Nico and Evan can provide good advice on how to do this but I've seen people
commit late at night and then revert so they can look at the performance numbers
on the bots.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (left):

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#old...
ipc/ipc_channel_posix.cc:765: return true;
You sure it's ok to remove this?

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:35: #endif  // defined (OS_MACOSX)
// OS_MACOSX

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:368: // Case 1
Could you add a bit of descriptive text to these rather than referring to them
just by number - would make things easier to follow. e.g.
// Case 1 - Channel wrapping a pipe.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:382: #endif   // defined(IPC_USES_READWRITE)
// IPC_USES_READWRITE

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:401: pipe_ = dup(pipe_);
HANDLE_EINTR(dup(pipe_));

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:459: #endif
// OS_MACOSX

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:474: #endif
// IPC_USES_READWRITE

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:983: // Close any outstanding, received file
descriptors
.

[dmac, 2010-12-13 18:32:29]

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (left):

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#old...
ipc/ipc_channel_posix.cc:765: return true;
On 2010/12/12 14:59:29, jeremy wrote:
> You sure it's ok to remove this?

Yep, it compiles cleanly without it.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:35: #endif  // defined (OS_MACOSX)
On 2010/12/12 14:59:29, jeremy wrote:
> // OS_MACOSX

Done.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:345: Mode mode) {
On 2010/12/10 18:08:03, Nico wrote:
> do you need the |mode| parameter?

Done.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:361: mode_ = MODE_SERVER;
On 2010/12/10 18:08:03, Nico wrote:
> |mode_| changing somewhere in the middle of this class feels a bit magical.
> Maybe you can keep this in the constructor and pass |uses_domain_socket| in as
> parameter?

Done.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:368: // Case 1
On 2010/12/12 14:59:29, jeremy wrote:
> Could you add a bit of descriptive text to these rather than referring to them
> just by number - would make things easier to follow. e.g.
> // Case 1 - Channel wrapping a pipe.

Done. Mapped them to the comment above to avoid duplicating the comments and
encouraging bitrot.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:382: #endif   // defined(IPC_USES_READWRITE)
On 2010/12/12 14:59:29, jeremy wrote:
> // IPC_USES_READWRITE

Done.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:401: pipe_ = dup(pipe_);
On 2010/12/12 14:59:29, jeremy wrote:
> HANDLE_EINTR(dup(pipe_));

Done.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:458: listening = count > 0 ? 0 : 1;
On 2010/12/10 18:48:07, Nico wrote:
> drop | ? 0 : 1|

Done.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:459: #endif
On 2010/12/12 14:59:29, jeremy wrote:
> // OS_MACOSX

Done.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:474: #endif
On 2010/12/12 14:59:29, jeremy wrote:
> // IPC_USES_READWRITE

Done.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:983: // Close any outstanding, received file
descriptors
On 2010/12/12 14:59:29, jeremy wrote:
> .

Done.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.cc#new...
ipc/ipc_channel_posix.cc:1116: unlink(pipe_name_.c_str());
On 2010/12/10 18:48:07, Nico wrote:
> Do you need to set must_link_ to false here? See comment about idempotency
> above.

Done.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.h
File ipc/ipc_channel_posix.h (right):

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix.h#newc...
ipc/ipc_channel_posix.h:138: // true if we responsible for unlinking the unix
domain socket file.
On 2010/12/10 18:08:03, Nico wrote:
> no verb

Done.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
File ipc/ipc_channel_posix_unittest.cc (right):

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
ipc/ipc_channel_posix_unittest.cc:22: static const int kIPCPosixTestTimeOutInMS
= 2000;
On 2010/12/10 18:48:15, Paweł Hajdan Jr. wrote:
> Please use base/test/test_timeouts.

Done. Thanks.. didn't know about those.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
ipc/ipc_channel_posix_unittest.cc:137: // in the case of a bad test.
On 2010/12/10 18:08:03, Nico wrote:
> "Usually, the run loop will quit sooner than that because all tests use a
> IPCChannelPosixTestListener which quits the current run loop on any channel
> activity."?
> 
> Is there some way you can enforce that this method isn't called without this
> listener in place?

Done.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
ipc/ipc_channel_posix_unittest.cc:137: // in the case of a bad test.
On 2010/12/10 18:08:03, Nico wrote:
> "Usually, the run loop will quit sooner than that because all tests use a
> IPCChannelPosixTestListener which quits the current run loop on any channel
> activity."?
> 
> Is there some way you can enforce that this method isn't called without this
> listener in place?

Done. But I don't know of any good way of enforcing it.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
ipc/ipc_channel_posix_unittest.cc:158: ASSERT_EQ(socketpair(AF_UNIX,
SOCK_STREAM, 0, pipe_fds), 0);
On 2010/12/10 18:08:03, Nico wrote:
> the argument order for ASSERT is expected, actual

Yes... which always bugs me ;-) Fixed.

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
ipc/ipc_channel_posix_unittest.cc:288: while (true) {
On 2010/12/10 18:48:15, Paweł Hajdan Jr. wrote:
> Why do we spin the loop multiple times? Can't we do it just once?

No, because some of the tests will return a variety of messages before we get to
MESSAGE_RECEIVED. In the cases above where we never get MESSAGE_RECEIVED, we
kill the test. Simplified it a bit though.

[Paweł Hajdan Jr., 2010-12-13 20:48:40]

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
File ipc/ipc_channel_posix_unittest.cc (right):

http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
ipc/ipc_channel_posix_unittest.cc:288: while (true) {
On 2010/12/13 18:32:29, dmac wrote:
> On 2010/12/10 18:48:15, Paweł Hajdan Jr. wrote:
> > Why do we spin the loop multiple times? Can't we do it just once?
> 
> No, because some of the tests will return a variety of messages before we get
to
> MESSAGE_RECEIVED. In the cases above where we never get MESSAGE_RECEIVED, we
> kill the test. Simplified it a bit though.

I still don't understand. It's the test quitting the loop, right?

If so, could you instead spin the loop only once, and check the condition before
quitting it, so we quit it only once?

[dmac, 2010-12-13 22:23:53]

On 2010/12/13 20:48:40, Paweł Hajdan Jr. wrote:
>
http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
> File ipc/ipc_channel_posix_unittest.cc (right):
> 
>
http://codereview.chromium.org/5749001/diff/4001/ipc/ipc_channel_posix_unitte...
> ipc/ipc_channel_posix_unittest.cc:288: while (true) {
> On 2010/12/13 18:32:29, dmac wrote:
> > On 2010/12/10 18:48:15, Paweł Hajdan Jr. wrote:
> > > Why do we spin the loop multiple times? Can't we do it just once?
> > 
> > No, because some of the tests will return a variety of messages before we
get
> to
> > MESSAGE_RECEIVED. In the cases above where we never get MESSAGE_RECEIVED, we
> > kill the test. Simplified it a bit though.
> 
> I still don't understand. It's the test quitting the loop, right?
> 
> If so, could you instead spin the loop only once, and check the condition
before
> quitting it, so we quit it only once?

Done.

[Paweł Hajdan Jr., 2010-12-14 08:34:13]

Code I commented in the drive-by LGTM. Thanks!

[dmaclach1, 2010-12-14 16:12:11]

On Tue, Dec 14, 2010 at 12:34 AM,  <phajdan.jr@chromium.org> wrote:
> Code I commented in the drive-by LGTM. Thanks!
>
> http://codereview.chromium.org/5749001/
>

NP. Thank you kindly for the review. Anybody else have any comments,
or can I submit this?

[Evan Martin, 2010-12-14 20:16:45]

Sorry, I really should have brought in agl from the start -- he is the author of
this code.

[dmaclach1, 2010-12-14 20:22:24]

agl, any comments? I know that you told me on a different review, that
you hadn't touched this code in a while, so I handed it off to evan.

[Evan Martin, 2010-12-14 20:23:18]

Did you consider the implementation strategy of using a subclass?  I feel this
named pipe stuff is a lot of code to be stuffing into this core class for this
edge case...

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix.cc#ne...
ipc/ipc_channel_posix.cc:303: pipe_name_(channel_handle.name),
Up until now, the channel name was just a process-local ID.  You are changing it
so that it now needs to be a path, and one properly scoped to the user-data-dir
of the current process.  I am worried there are places in our code where we
don't obey this.

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix_unitt...
File ipc/ipc_channel_posix_unittest.cc (right):

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix_unitt...
ipc/ipc_channel_posix_unittest.cc:18: #include "ipc/ipc_channel_posix.h"
This should be in its own block around line 6 I think.

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_win.cc
File ipc/ipc_channel_win.cc (right):

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_win.cc#newc...
ipc/ipc_channel_win.cc:122: // Intentionally no default case here so that the
compiler
I think this comment is superfluous, but it's your call.

[agl, 2010-12-14 20:31:36]

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix.cc#ne...
ipc/ipc_channel_posix.cc:434: // We support sockets that we are connected to and
listening to.
This line is unclear. A socket cannot be both connected and listening.

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix.cc#ne...
ipc/ipc_channel_posix.cc:435: // Currently we only support one connection per
socket that we are
"one connection per listening socket"

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix.cc#ne...
ipc/ipc_channel_posix.cc:439: #if !defined(OS_MACOSX)
This code is terrible! Is there a case where you get passed a file descriptor
but don't know if it's intended to be listening or not? If so, then you need to
add an argument.

[dmac, 2010-12-14 21:01:16]

Thanks for taking a look guys.

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix.cc#ne...
ipc/ipc_channel_posix.cc:303: pipe_name_(channel_handle.name),
On 2010/12/14 20:23:19, Evan Martin wrote:
> Up until now, the channel name was just a process-local ID.  You are changing
it
> so that it now needs to be a path, and one properly scoped to the
user-data-dir
> of the current process.  I am worried there are places in our code where we
> don't obey this.

It only needs to be a path in the MODE_NAMED_SERVER or the MODE_NAMED_CLIENT
cases. We are only using this right now for services and automation. Other cases
don't need to change.

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix.cc#ne...
ipc/ipc_channel_posix.cc:434: // We support sockets that we are connected to and
listening to.
On 2010/12/14 20:31:36, agl wrote:
> This line is unclear. A socket cannot be both connected and listening.

Removed.

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix.cc#ne...
ipc/ipc_channel_posix.cc:435: // Currently we only support one connection per
socket that we are
On 2010/12/14 20:31:36, agl wrote:
> "one connection per listening socket"

Removed.

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix.cc#ne...
ipc/ipc_channel_posix.cc:439: #if !defined(OS_MACOSX)
On 2010/12/14 20:31:36, agl wrote:
> This code is terrible! Is there a case where you get passed a file descriptor
> but don't know if it's intended to be listening or not? If so, then you need
to
> add an argument.

Good call. Sorry. This was a left over piece of code from an earlier design.

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix_unitt...
File ipc/ipc_channel_posix_unittest.cc (right):

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_posix_unitt...
ipc/ipc_channel_posix_unittest.cc:18: #include "ipc/ipc_channel_posix.h"
On 2010/12/14 20:23:19, Evan Martin wrote:
> This should be in its own block around line 6 I think.

I was matching the style of the other unittest files in this subproject. I'll
change it over.

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_win.cc
File ipc/ipc_channel_win.cc (right):

http://codereview.chromium.org/5749001/diff/29001/ipc/ipc_channel_win.cc#newc...
ipc/ipc_channel_win.cc:122: // Intentionally no default case here so that the
compiler
On 2010/12/14 20:23:19, Evan Martin wrote:
> I think this comment is superfluous, but it's your call.

I'm going to leave it as I have had other devs ask me why I had a switch with no
default.

[agl, 2010-12-14 21:04:14]

LGTM

(The IPC is turning into rather a disaster, but that's not your fault.)

[dmaclach1, 2010-12-14 21:08:23]

thanks AGL...

I was actually trying hard to not make it more of a disaster than it was ;-)

On Tue, Dec 14, 2010 at 1:04 PM,  <agl@chromium.org> wrote:
> LGTM
>
> (The IPC is turning into rather a disaster, but that's not your fault.)
>
> http://codereview.chromium.org/5749001/
>

[dmac, 2010-12-16 22:11:17]

This patch had some troubles with valgrind and timing out. I also tightened up
some of the unittesting checks.