Wait on a pipe for the test server to start up

Wait on a pipe for the test server to start up

This should speed up testserver-based unit tests considerably
and make them less flakey.

R=agl,cpu,phajdan,wtc
BUG=49680
TEST=net_unittests

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

[davidben, 2010-09-12 06:49:16]

Pawel: I don't know if you ended up working on this, but I thought I would give
it a go. It opens a pipe and passes the file descriptor (or file handle on
Windows) as a command-line option.

(Note: I don't have Windows and Mac build environments set up yet, so my tests
on those platforms have been entirely by try server.)

By completely unscientific comparisons (picking random other try server runs to
compare against) of this CL and http://codereview.chromium.org/3322021/show , I
get timings of:

Linux:
unchanged: 151s [1]
with CL: 85s [2]
with CL and #3322021: 84s [3]

Mac OS X:
unchanged: 190s [4]
with CL: 176s [5]
with CL and #3322021: 146s [6]

Windows:
unchanged: 639s [7]
with CL: 117s [8]

[1]
http://build.chromium.org/buildbot/try-server/builders/linux/builds/46163/ste...
[2]
http://build.chromium.org/buildbot/try-server/builders/linux/builds/46164/ste...
[3]
http://build.chromium.org/buildbot/try-server/builders/linux/builds/46168/ste...

[4]
http://build.chromium.org/buildbot/try-server/builders/mac/builds/46113/steps...
[5]
http://build.chromium.org/buildbot/try-server/builders/mac/builds/46114/steps...
[6]
http://build.chromium.org/buildbot/try-server/builders/mac/builds/46117/steps...

[7]
http://build.chromium.org/buildbot/try-server/builders/win/builds/49221/steps...
[8]
http://build.chromium.org/buildbot/try-server/builders/win/builds/49227/steps...

I suspect the lack of differences between [2] and [3] may be that the try
servers don't have twisted installed? Or maybe fluctuations. I don't know how
much these timings vary. (On my machine, I get a fairly noticeable difference.)

[Paweł Hajdan Jr., 2010-09-13 17:26:34]

+agl for POSIX code
+cpu for Windows code

David, this patch looks great to me. I'm really excited to see this work.

LGTM (of course please wait for agl's and cpu's review, I don't really
understand some of the changes, especially the Windows code).

http://codereview.chromium.org/3368012/diff/2001/3004
File net/test/test_server_win.cc (right):

http://codereview.chromium.org/3368012/diff/2001/3004#newcode178
net/test/test_server_win.cc:178: 
nit: remove this empty line.

[agl, 2010-09-13 17:47:35]

POSIX LGTM

[wtc, 2010-09-15 00:58:29]

LGTM.  Excellent work!

Some suggested changes below.  Note especially the comment
marked "DESIGN ISSUE".

http://codereview.chromium.org/3368012/diff/2001/3001
File net/test/test_server.cc (left):

http://codereview.chromium.org/3368012/diff/2001/3001#oldcode245
net/test/test_server.cc:245: net::TCPPinger pinger(addr);
Can you check if we remove the TCPPinger class now?

http://codereview.chromium.org/3368012/diff/2001/3002
File net/test/test_server.h (right):

http://codereview.chromium.org/3368012/diff/2001/3002#newcode117
net/test/test_server.h:117: // File handle of the child writes to when it
starts.
Nit: File handle of => The file handle

http://codereview.chromium.org/3368012/diff/2001/3003
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/3368012/diff/2001/3003#newcode52
net/test/test_server_posix.cc:52: if (pipe(pipefd) < 0) {
Nit: I usually use != 0 instead of < 0 or == -1 in this
situation, with the assumption that processors usually have
instructions for zero/non-zero comparisons.

http://codereview.chromium.org/3368012/diff/2001/3003#newcode53
net/test/test_server_posix.cc:53: PLOG(ERROR) << "Could not create pipe.";
Should log 'errno'.

Why do you use PLOG instead of LOG here?

http://codereview.chromium.org/3368012/diff/2001/3003#newcode75
net/test/test_server_posix.cc:75: char buf[64];
Since we're just reading "READY", 'buf' can be just 8 bytes.

http://codereview.chromium.org/3368012/diff/2001/3004
File net/test/test_server_win.cc (right):

http://codereview.chromium.org/3368012/diff/2001/3004#newcode134
net/test/test_server_win.cc:134: PLOG(ERROR) << "Failed to create pipe";
Why do you use PLOG instead of LOG here at at line 142?
(I don't know what PLOG is...)

http://codereview.chromium.org/3368012/diff/2001/3004#newcode140
net/test/test_server_win.cc:140: // Don't inherit the reading handle.
Nit: this should say
  Don't let the child inherit the reading handle.

http://codereview.chromium.org/3368012/diff/2001/3005
File net/tools/testserver/testserver.py (left):

http://codereview.chromium.org/3368012/diff/2001/3005#oldcode1216
net/tools/testserver/testserver.py:1216: 
Nit: it seems nice to keep this blank line...

http://codereview.chromium.org/3368012/diff/2001/3005
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/3368012/diff/2001/3005#newcode1305
net/tools/testserver/testserver.py:1305: startup_file = os.fdopen(fd, "w")
Nit: startup_file => startup_pipe

Can Python write to a Win32 file handle or a Unix file
descriptor directly?  Do we really need to convert them
to a FILE*?

http://codereview.chromium.org/3368012/diff/2001/3005#newcode1343
net/tools/testserver/testserver.py:1343: option_parser.add_option('',
'--startup-handle', type='int',
DESIGN ISSUE: do we make the "handle" vs. "fd" distinction
elsewhere?

We should just use --startup-fd on all platforms.

[davidben, 2010-09-15 02:41:01]

http://codereview.chromium.org/3368012/diff/2001/3001
File net/test/test_server.cc (left):

http://codereview.chromium.org/3368012/diff/2001/3001#oldcode245
net/test/test_server.cc:245: net::TCPPinger pinger(addr);
On 2010/09/15 00:58:29, wtc wrote:
> Can you check if we remove the TCPPinger class now?

Seems so. Removed.

http://codereview.chromium.org/3368012/diff/2001/3002
File net/test/test_server.h (right):

http://codereview.chromium.org/3368012/diff/2001/3002#newcode117
net/test/test_server.h:117: // File handle of the child writes to when it
starts.
On 2010/09/15 00:58:29, wtc wrote:
> Nit: File handle of => The file handle

Done.

http://codereview.chromium.org/3368012/diff/2001/3003
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/3368012/diff/2001/3003#newcode52
net/test/test_server_posix.cc:52: if (pipe(pipefd) < 0) {
On 2010/09/15 00:58:29, wtc wrote:
> Nit: I usually use != 0 instead of < 0 or == -1 in this
> situation, with the assumption that processors usually have
> instructions for zero/non-zero comparisons.

Done.

http://codereview.chromium.org/3368012/diff/2001/3003#newcode53
net/test/test_server_posix.cc:53: PLOG(ERROR) << "Could not create pipe.";
On 2010/09/15 00:58:29, wtc wrote:
> Should log 'errno'.
> 
> Why do you use PLOG instead of LOG here?

Because PLOG will log errno for me. :-D

http://codereview.chromium.org/3368012/diff/2001/3003#newcode75
net/test/test_server_posix.cc:75: char buf[64];
On 2010/09/15 00:58:29, wtc wrote:
> Since we're just reading "READY", 'buf' can be just 8 bytes.

Done.

http://codereview.chromium.org/3368012/diff/2001/3004
File net/test/test_server_win.cc (right):

http://codereview.chromium.org/3368012/diff/2001/3004#newcode134
net/test/test_server_win.cc:134: PLOG(ERROR) << "Failed to create pipe";
On 2010/09/15 00:58:29, wtc wrote:
> Why do you use PLOG instead of LOG here at at line 142?
> (I don't know what PLOG is...)

It appends a string form of GetLastError()/errno.

http://codereview.chromium.org/3368012/diff/2001/3004#newcode140
net/test/test_server_win.cc:140: // Don't inherit the reading handle.
On 2010/09/15 00:58:29, wtc wrote:
> Nit: this should say
>   Don't let the child inherit the reading handle.

Done.

http://codereview.chromium.org/3368012/diff/2001/3004#newcode178
net/test/test_server_win.cc:178: 
On 2010/09/13 17:26:34, Paweł Hajdan Jr. wrote:
> nit: remove this empty line.

Done.

http://codereview.chromium.org/3368012/diff/2001/3005
File net/tools/testserver/testserver.py (left):

http://codereview.chromium.org/3368012/diff/2001/3005#oldcode1216
net/tools/testserver/testserver.py:1216: 
On 2010/09/15 00:58:29, wtc wrote:
> Nit: it seems nice to keep this blank line...

Oops. Yeah, stray edit. Done.

http://codereview.chromium.org/3368012/diff/2001/3005
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/3368012/diff/2001/3005#newcode1305
net/tools/testserver/testserver.py:1305: startup_file = os.fdopen(fd, "w")
On 2010/09/15 00:58:29, wtc wrote:
> Nit: startup_file => startup_pipe

Done.

> Can Python write to a Win32 file handle or a Unix file
> descriptor directly?  Do we really need to convert them
> to a FILE*?

Python can write to a file descriptor directly (os.write). I don't think it can
write to a file handle (except via msvcrt.open_osfhandle). I figured I may as
well wrap it in a FILE* since it'd take care of buffering and everything, in
case we cared more about what got written to the pipe later.

http://codereview.chromium.org/3368012/diff/2001/3005#newcode1343
net/tools/testserver/testserver.py:1343: option_parser.add_option('',
'--startup-handle', type='int',
On 2010/09/15 00:58:29, wtc wrote:
> DESIGN ISSUE: do we make the "handle" vs. "fd" distinction
> elsewhere?

I'm not aware of other code that passes around fds/handles this way.

> We should just use --startup-fd on all platforms.

Should we? I thought it would be good to be specific on Windows at least as it
appears to have both notions. There's file handles and file descriptors which I
gather are (uninheritable?) C run-time wrappers allocated by _open_osfhandle. I
thought it'd be good to distinguish in this case. (Or I could be confused. I
don't actually know anything about Win32 and did a lot of documentation-diving
to write this CL.)

I'm actually somewhat annoyed that dest is startup_fd instead of startup_handle,
but it otherwise makes the checking code a little more annoying.


Not all that attached to this though. Shrug. I could also change it to handle
everywhere, as I think handle and fd would be synonymous in this context on
UNIX.

[Paweł Hajdan Jr., 2010-09-15 17:35:52]

Yay LGTM!

About wtc's comment (file handle vs. fd in the switch name) - I don't have a
strong preference for either option. I'm not sure if it's worth changing the
switch name for that.

[wtc, 2010-09-15 17:55:14]

LGTM.

Thank you for explaining what PLOG is.  I learned something new.

http://codereview.chromium.org/3368012/diff/12001/11006
File net/test/test_server.h (right):

http://codereview.chromium.org/3368012/diff/12001/11006#newcode118
net/test/test_server.h:118: ScopedHandle child_fd_;
You're right about 'fd' on Win32.  It is not a native Win32
concept, but the CRT implements 'fd' for POSIX compatiblity.
See, for example,
http://msdn.microsoft.com/en-us/library/1570wh78(VS.80).aspx

So using 'fd' for Win32 could confuse someone who knows a
lot about these things.

But I think for simplicity it is fine to be imprecise and
use the same term for both Unix and Windows.  For example,
you use child_fd_ for the file handle here.

Another idea is to use a neutral term in the command line option: 
--startup-pipe.  What do you think?

In any case, let's not allow the greater good get bogged down
by this naming issue.  I have a slight preference for using
the same command-line option between Unix and Windows, but
the current CL is just fine, too.

[cpu_(ooo_6.6-7.5), 2010-09-16 23:14:20]

lgtm, see my optional comment.

http://codereview.chromium.org/3368012/diff/12001/11008
File net/test/test_server_win.cc (right):

http://codereview.chromium.org/3368012/diff/12001/11008#newcode136
net/test/test_server_win.cc:136: }
You can also remove lines 128 to 132 and use
SetHandleInformation(child_write,HANDLE_FLAG_INHERIT, 1) instead. Seems shorter.

[wtc, 2010-09-17 18:11:48]

http://codereview.chromium.org/3368012/diff/12001/11008
File net/test/test_server_win.cc (right):

http://codereview.chromium.org/3368012/diff/12001/11008#newcode136
net/test/test_server_win.cc:136: }
On 2010/09/16 23:14:20, cpu wrote:
> You can also remove lines 128 to 132 and use
> SetHandleInformation(child_write,HANDLE_FLAG_INHERIT, 1) instead. Seems
shorter.

I also considered suggesting this change, actually :-)

It's better to use the symbolic constant
HANDLE_FLAG_INHERIT instead of 1 as the third argument.

davidben: if you make this change, you should remove
lines 140-144 because if you pass NULL as the third
argument to CreatePipe, the pipe handles cannot be
inherited.

[davidben, 2010-09-18 21:43:01]

http://codereview.chromium.org/3368012/diff/12001/11006
File net/test/test_server.h (right):

http://codereview.chromium.org/3368012/diff/12001/11006#newcode118
net/test/test_server.h:118: ScopedHandle child_fd_;
On 2010/09/15 17:55:14, wtc wrote:
> Another idea is to use a neutral term in the command line option: 
> --startup-pipe.  What do you think?

Sounds good. Done.

http://codereview.chromium.org/3368012/diff/12001/11008
File net/test/test_server_win.cc (right):

http://codereview.chromium.org/3368012/diff/12001/11008#newcode136
net/test/test_server_win.cc:136: }
On 2010/09/17 18:11:49, wtc wrote:
> On 2010/09/16 23:14:20, cpu wrote:
> > You can also remove lines 128 to 132 and use
> > SetHandleInformation(child_write,HANDLE_FLAG_INHERIT, 1) instead. Seems
> shorter.
> 
> I also considered suggesting this change, actually :-)
> 
> It's better to use the symbolic constant
> HANDLE_FLAG_INHERIT instead of 1 as the third argument.
> 
> davidben: if you make this change, you should remove
> lines 140-144 because if you pass NULL as the third
> argument to CreatePipe, the pipe handles cannot be
> inherited.

Done.

[wtc, 2010-09-19 00:09:25]

LGTM.