Fix NamedProxyLauncher on Windows.

Fix NamedProxyLauncher on windows. Wait for the named pipe to be connectable.
Enable NamedInterfaceTest on windows.
BUG=chromium-os:8515
TEST=none


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

[kkania, 2011-07-21 22:10:41]

jam: ipc/*
dtu: all else

[kkania, 2011-07-21 22:12:56]

tsepez: ipc/* (jam's out)

[dtu, 2011-07-21 22:33:38]

Thanks for doing this Ken!

LGTM

[Tom Sepez, 2011-07-21 23:43:20]

The proxy launcher has too much knowledge of the platform specific
implementation details of the channel.  What's required is a way for it to ask
IPC whether a given channel_id is ready, rather than poking around itself. 

I'd like to get John's take on what such an interface would look like.  Is there
a lot of time pressure on this?

[kkania, 2011-07-21 23:48:49]

This can wait till John gets back if you would like.

What do you think of adding a static method
Channel::IsChannelAvailableForConnection which does this check?

[Tom Sepez, 2011-07-21 23:55:38]

On 2011/07/21 23:48:49, kkania wrote:
> This can wait till John gets back if you would like.
> 
> What do you think of adding a static method
> Channel::IsChannelAvailableForConnection which does this check?

That would be my suggestion, but I'd call it Channel::IsAvailableForConnection
(without repeating "Channel").   But check with John before you spend any more
time on it.

[kkania, 2011-07-25 22:09:57]

Adding John as reviewer, as requested by Tom.

[kkania, 2011-07-26 22:48:07]

reminder to jam this is awaiting his input

[jam, 2011-07-27 16:59:43]

I'm not the best reviewer for this, since that code was written before I started
and I haven't touched it much. I think Darin might have written it.

If he's fine with this, the one nit I'll make is that PipeName should still be
in the class, just be a static method instead.

[kkania, 2011-07-28 23:11:48]

-jam, +darin

Darin,
The current plan for this CL is to add:
static bool Channel::IsAvailableForConnection(channel_id),
which returns whether a server channel is listening on that channel ID.

One other piece I would like to add to this CL is fixing the race where the
ProxyLauncher sees the domain socket file exists and attempts to connect before
the server calls listen.
One hacky way to fix this would be to create the domain socket with a temp file
and only after calling listen, rename it to the actual pipe name.

What do you think?

[kkania, 2011-08-01 23:09:40]

+dmaclach

Seems Darin is busy. Do you think you can comment/review here instead?

On 2011/07/28 23:11:48, kkania wrote:
> -jam, +darin
> 
> Darin,
> The current plan for this CL is to add:
> static bool Channel::IsAvailableForConnection(channel_id),
> which returns whether a server channel is listening on that channel ID.
> 
> One other piece I would like to add to this CL is fixing the race where the
> ProxyLauncher sees the domain socket file exists and attempts to connect
before
> the server calls listen.
> One hacky way to fix this would be to create the domain socket with a temp
file
> and only after calling listen, rename it to the actual pipe name.
> 
> What do you think?

[dmac, 2011-08-02 00:32:04]

On 2011/08/01 23:09:40, kkania wrote:
> +dmaclach
> 
> Seems Darin is busy. Do you think you can comment/review here instead?
> 
> On 2011/07/28 23:11:48, kkania wrote:
> > -jam, +darin
> > 
> > Darin,
> > The current plan for this CL is to add:
> > static bool Channel::IsAvailableForConnection(channel_id),
> > which returns whether a server channel is listening on that channel ID.
> > 
> > One other piece I would like to add to this CL is fixing the race where the
> > ProxyLauncher sees the domain socket file exists and attempts to connect
> before
> > the server calls listen.
> > One hacky way to fix this would be to create the domain socket with a temp
> file
> > and only after calling listen, rename it to the actual pipe name.
> > 
> > What do you think?

I somewhat agree with Tom's initial assessment, that we should be asking the
channel if it is ready instead of poking around in it. This would also have the
advantage of getting rid of the #ifdef block in proxy_launcher and moving it
into the already split up platform implementations of IPC::Channel.

[dmac, 2011-08-02 00:32:13]

http://codereview.chromium.org/7486007/diff/2001/chrome/test/automation/proxy...
File chrome/test/automation/proxy_launcher.cc (right):

http://codereview.chromium.org/7486007/diff/2001/chrome/test/automation/proxy...
chrome/test/automation/proxy_launcher.cc:544: 1);
on Windows does it not make more sense just to call WaitNamedPipe(name,
TestTimeouts::action_max_timout_ms())?

[kkania, 2011-08-02 15:00:58]

dmac,

Thanks for the comment. I'll put up a change and let you review in place of
Darin, if that's ok.

http://codereview.chromium.org/7486007/diff/2001/chrome/test/automation/proxy...
File chrome/test/automation/proxy_launcher.cc (right):

http://codereview.chromium.org/7486007/diff/2001/chrome/test/automation/proxy...
chrome/test/automation/proxy_launcher.cc:544: 1);
WaitNamedPipe on windows returns immediately if the pipe doesn't exist yet. The
wait param supposedly only comes into play when waiting for an existing pipe to
be available for connection

[dmaclach1, 2011-08-02 15:17:06]

On Tue, Aug 2, 2011 at 08:00,  <kkania@chromium.org> wrote:
> dmac,
>
> Thanks for the comment. I'll put up a change and let you review in place of
> Darin, if that's ok.
>
>
>
http://codereview.chromium.org/7486007/diff/2001/chrome/test/automation/proxy...
> File chrome/test/automation/proxy_launcher.cc (right):
>
>
http://codereview.chromium.org/7486007/diff/2001/chrome/test/automation/proxy...
> chrome/test/automation/proxy_launcher.cc:544: 1);
> WaitNamedPipe on windows returns immediately if the pipe doesn't exist
> yet. The wait param supposedly only comes into play when waiting for an
> existing pipe to be available for connection

and does 0 signify wait forever then? Just wondering why 1 instead of 0.

[kkania, 2011-08-02 18:26:35]

added IsNamedServerInitialized static method to Channel. I named it this instead
of IsAvailableForConnection because on POSIX we do not know if it is actually
available, just if it is initialized

dmac: passing 0 to WaitNamedPipe uses the default timeout, which was set at 5s
in CreateNamedPipe.

[kkania, 2011-08-05 23:07:31]

ping. Waiting for dmac

[dmac, 2011-08-05 23:10:23]

On 2011/08/05 23:07:31, kkania wrote:
> ping. Waiting for dmac

LGTM

[commit-bot: I haz the power, 2011-08-09 06:46:07]

Change committed as 95957