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

Review URL: http://codereview.chromium.org/3368012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@60199 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 90 insertions, 262 deletions

diff --git a/net/net.gyp b/net/net.gyp
index 2358473..4788816 100644
--- a/net/net.gyp
+++ b/net/net.gyp
@@ -549,7 +549,6 @@
         'socket/tcp_client_socket_pool.h',
         'socket/tcp_client_socket_win.cc',
         'socket/tcp_client_socket_win.h',
-        'socket/tcp_pinger.h',
         'socket_stream/socket_stream.cc',
         'socket_stream/socket_stream.h',
         'socket_stream/socket_stream_job.cc',
@@ -839,7 +838,6 @@
         'socket/ssl_client_socket_pool_unittest.cc',
         'socket/tcp_client_socket_pool_unittest.cc',
         'socket/tcp_client_socket_unittest.cc',
-        'socket/tcp_pinger_unittest.cc',
         'socket_stream/socket_stream_metrics_unittest.cc',
         'socket_stream/socket_stream_unittest.cc',
         'spdy/spdy_framer_test.cc',
diff --git a/net/socket/tcp_pinger.h b/net/socket/tcp_pinger.h
deleted file mode 100644
index 8b0f416..0000000
--- a/net/socket/tcp_pinger.h
+++ /dev/null
@@ -1,141 +0,0 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef NET_SOCKET_TCP_PINGER_H_
-#define NET_SOCKET_TCP_PINGER_H_
-#pragma once
-
-#include "base/compiler_specific.h"
-#include "base/ref_counted.h"
-#include "base/scoped_ptr.h"
-#include "base/task.h"
-#include "base/third_party/dynamic_annotations/dynamic_annotations.h"
-#include "base/thread.h"
-#include "base/waitable_event.h"
-#include "net/base/address_list.h"
-#include "net/base/completion_callback.h"
-#include "net/base/net_errors.h"
-#include "net/socket/tcp_client_socket.h"
-
-namespace base {
-class TimeDelta;
-}
-
-namespace net {
-
-// Simple class to wait until a TCP server is accepting connections.
-class TCPPinger {
- public:
-  explicit TCPPinger(const net::AddressList& addr)
-    : io_thread_("TCPPinger"),
-      worker_(new Worker(addr)) {
-    worker_->AddRef();
-    // Start up a throwaway IO thread just for this.
-    // TODO(dkegel): use some existing thread pool instead?
-    base::Thread::Options options;
-    options.message_loop_type = MessageLoop::TYPE_IO;
-    io_thread_.StartWithOptions(options);
-  }
-
-  ~TCPPinger() {
-    io_thread_.message_loop()->ReleaseSoon(FROM_HERE, worker_);
-  }
-
-  int Ping() {
-    // Default is 10 tries, each with a timeout of 1000ms,
-    // for a total max timeout of 10 seconds.
-    return Ping(base::TimeDelta::FromMilliseconds(1000), 10);
-  }
-
-  int Ping(base::TimeDelta tryTimeout, int nTries) {
-    int err = ERR_IO_PENDING;
-    // Post a request to do the connect on that thread.
-    for (int i = 0; i < nTries; i++) {
-      io_thread_.message_loop()->PostTask(FROM_HERE,
-        NewRunnableMethod(worker_,
-        &net::TCPPinger::Worker::DoConnect));
-      // Timeout here in case remote host offline
-      err = worker_->TimedWaitForResult(tryTimeout);
-      if (err == net::OK)
-        break;
-      PlatformThread::Sleep(static_cast<int>(tryTimeout.InMilliseconds()));
-
-      // Cancel leftover activity, if any
-      io_thread_.message_loop()->PostTask(FROM_HERE,
-        NewRunnableMethod(worker_,
-        &net::TCPPinger::Worker::DoDisconnect));
-      worker_->WaitForResult();
-    }
-    return err;
-  }
-
- private:
-
-  // Inner class to handle all actual socket calls.
-  // This makes the outer interface simpler,
-  // and helps us obey the "all socket calls
-  // must be on same thread" restriction.
-  class Worker : public base::RefCountedThreadSafe<Worker> {
-   public:
-    explicit Worker(const net::AddressList& addr)
-      : event_(false, false),
-        net_error_(ERR_IO_PENDING),
-        addr_(addr),
-        ALLOW_THIS_IN_INITIALIZER_LIST(connect_callback_(this,
-            &net::TCPPinger::Worker::ConnectDone)) {
-    }
-
-    void DoConnect() {
-      sock_.reset(new TCPClientSocket(addr_, NULL, net::NetLog::Source()));
-      int rv = sock_->Connect(&connect_callback_);
-      // Regardless of success or failure, if we're done now,
-      // signal the customer.
-      if (rv != ERR_IO_PENDING)
-        ConnectDone(rv);
-    }
-
-    void DoDisconnect() {
-      sock_.reset();
-      event_.Signal();
-    }
-
-    void ConnectDone(int rv) {
-      sock_.reset();
-      net_error_ = rv;
-      event_.Signal();
-    }
-
-    int TimedWaitForResult(base::TimeDelta tryTimeout) {
-      event_.TimedWait(tryTimeout);
-      // In case of timeout, the value of net_error_ should be ERR_IO_PENDING.
-      // However, a harmless data race can happen if TimedWait times out right
-      // before event_.Signal() is called in ConnectDone().
-      return ANNOTATE_UNPROTECTED_READ(net_error_);
-    }
-
-    int WaitForResult() {
-      event_.Wait();
-      return net_error_;
-    }
-
-   private:
-    friend class base::RefCountedThreadSafe<Worker>;
-
-    ~Worker() {}
-
-    base::WaitableEvent event_;
-    int net_error_;
-    net::AddressList addr_;
-    scoped_ptr<TCPClientSocket> sock_;
-    net::CompletionCallbackImpl<Worker> connect_callback_;
-  };
-
-  base::Thread io_thread_;
-  Worker* worker_;
-  DISALLOW_COPY_AND_ASSIGN(TCPPinger);
-};
-
-}  // namespace net
-
-#endif  // NET_SOCKET_TCP_PINGER_H_
diff --git a/net/socket/tcp_pinger_unittest.cc b/net/socket/tcp_pinger_unittest.cc
deleted file mode 100644
index 3662032..0000000
--- a/net/socket/tcp_pinger_unittest.cc
+++ /dev/null
@@ -1,99 +0,0 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "base/ref_counted.h"
-#include "base/trace_event.h"
-#include "net/base/address_list.h"
-#include "net/base/host_resolver.h"
-#include "net/base/listen_socket.h"
-#include "net/base/net_errors.h"
-#include "net/base/winsock_init.h"
-#include "net/socket/tcp_client_socket.h"
-#include "net/socket/tcp_pinger.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "testing/platform_test.h"
-
-class TCPPingerTest
-    : public PlatformTest, public ListenSocket::ListenSocketDelegate {
- public:
-  TCPPingerTest() : listen_port_(0) {
-  }
-
-  // Implement ListenSocketDelegate methods
-  virtual void DidAccept(ListenSocket* server, ListenSocket* connection) {
-    // This callback doesn't seem to happen
-    // right away, so this handler may not be called at all
-    // during connect-only tests.
-    LOG(INFO) << "TCPPinger accepted connection";
-    connected_sock_ = connection;
-  }
-  virtual void DidRead(ListenSocket*, const char* str, int len) {
-    // Not really needed yet, as TCPPinger doesn't support Read
-    connected_sock_->Send(std::string("HTTP/1.1 404 Not Found"), true);
-    connected_sock_ = NULL;
-  }
-  virtual void DidClose(ListenSocket* sock) {}
-
-  // Testcase hooks
-  virtual void SetUp();
-
- protected:
-  int listen_port_;
-  scoped_refptr<ListenSocket> listen_sock_;
-
- private:
-  scoped_refptr<ListenSocket> connected_sock_;
-};
-
-void TCPPingerTest::SetUp() {
-  PlatformTest::SetUp();
-
-  // Find a free port to listen on
-  // Range of ports to listen on.  Shouldn't need to try many.
-  static const int kMinPort = 10100;
-  static const int kMaxPort = 10200;
-#if defined(OS_WIN)
-  net::EnsureWinsockInit();
-#endif
-  for (listen_port_ = kMinPort; listen_port_ < kMaxPort; listen_port_++) {
-    listen_sock_ = ListenSocket::Listen("127.0.0.1", listen_port_, this);
-    if (listen_sock_.get()) break;
-  }
-  ASSERT_TRUE(listen_sock_.get() != NULL);
-}
-
-TEST_F(TCPPingerTest, Ping) {
-  net::AddressList addr;
-  scoped_refptr<net::HostResolver> resolver(
-      net::CreateSystemHostResolver(net::HostResolver::kDefaultParallelism,
-                                    NULL));
-
-  net::HostResolver::RequestInfo info(
-      net::HostPortPair("localhost", listen_port_));
-  int rv = resolver->Resolve(info, &addr, NULL, NULL, net::BoundNetLog());
-  EXPECT_EQ(rv, net::OK);
-
-  net::TCPPinger pinger(addr);
-  rv = pinger.Ping();
-  EXPECT_EQ(rv, net::OK);
-}
-
-TEST_F(TCPPingerTest, PingFail) {
-  net::AddressList addr;
-  scoped_refptr<net::HostResolver> resolver(
-      net::CreateSystemHostResolver(net::HostResolver::kDefaultParallelism,
-                                    NULL));
-
-  // "Kill" "server"
-  listen_sock_ = NULL;
-
-  net::HostResolver::RequestInfo info(
-      net::HostPortPair("localhost", listen_port_));
-  int rv = resolver->Resolve(info, &addr, NULL, NULL, net::BoundNetLog());
-  EXPECT_EQ(rv, net::OK);
-
-  net::TCPPinger pinger(addr);
-  rv = pinger.Ping(base::TimeDelta::FromMilliseconds(100), 1);
-  EXPECT_NE(rv, net::OK);
-}
diff --git a/net/test/test_server.cc b/net/test/test_server.cc
index 4c698b0..536d351 100644
--- a/net/test/test_server.cc
+++ b/net/test/test_server.cc
@@ -27,7 +27,6 @@
 #include "net/base/host_resolver.h"
 #include "net/base/test_completion_callback.h"
 #include "net/socket/tcp_client_socket.h"
-#include "net/socket/tcp_pinger.h"
 #include "net/test/python_utils.h"
 #include "testing/platform_test.h"
 
@@ -256,22 +255,6 @@ bool TestServer::SetPythonPath() {
   return true;
 }
 
-bool TestServer::WaitToStart() {
-  net::AddressList addr;
-  if (!GetAddressList(&addr))
-    return false;
-
-  net::TCPPinger pinger(addr);
-  int rv = pinger.Ping(
-      base::TimeDelta::FromMilliseconds(kServerConnectionTimeoutMs),
-      kServerConnectionAttempts);
-  bool result = (rv == net::OK);
-  if (!result) {
-    LOG(ERROR) << "Failed to connect to server";
-  }
-  return result;
-}
-
 FilePath TestServer::GetRootCertificatePath() {
   return certificates_dir_.AppendASCII("root_ca_cert.crt");
 }
diff --git a/net/test/test_server.h b/net/test/test_server.h
index 8b6dc63..6baaaec 100644
--- a/net/test/test_server.h
+++ b/net/test/test_server.h
@@ -12,6 +12,7 @@
 
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
+#include "base/file_util.h"
 #include "base/process_util.h"
 #include "net/base/host_port_pair.h"
 
@@ -109,6 +110,15 @@ class TestServer {
 #if defined(OS_WIN)
   // JobObject used to clean up orphaned child processes.
   ScopedHandle job_handle_;
+
+  // The file handle the child writes to when it starts.
+  ScopedHandle child_fd_;
+#endif
+
+#if defined(OS_POSIX)
+  // The file descriptor the child writes to when it starts.
+  int child_fd_;
+  file_util::ScopedFD child_fd_closer_;
 #endif
 
 #if defined(USE_NSS)
diff --git a/net/test/test_server_posix.cc b/net/test/test_server_posix.cc
index 7741d89..262845c 100644
--- a/net/test/test_server_posix.cc
+++ b/net/test/test_server_posix.cc
@@ -32,8 +32,22 @@ bool TestServer::LaunchPython(const FilePath& testserver_path) {
   if (type_ == TYPE_HTTPS_CLIENT_AUTH)
     command_line.push_back("--ssl-client-auth");
 
-  base::file_handle_mapping_vector no_mappings;
-  if (!base::LaunchApp(command_line, no_mappings, false, &process_handle_)) {
+  int pipefd[2];
+  if (pipe(pipefd) != 0) {
+    PLOG(ERROR) << "Could not create pipe.";
+    return false;
+  }
+
+  // Save the read half. The write half is sent to the child.
+  child_fd_ = pipefd[0];
+  child_fd_closer_.reset(&child_fd_);
+  file_util::ScopedFD write_closer(&pipefd[1]);
+  base::file_handle_mapping_vector map_write_fd;
+  map_write_fd.push_back(std::make_pair(pipefd[1], pipefd[1]));
+
+  command_line.push_back("--startup-pipe=" + base::IntToString(pipefd[1]));
+
+  if (!base::LaunchApp(command_line, map_write_fd, false, &process_handle_)) {
     LOG(ERROR) << "Failed to launch " << command_line[0] << " ...";
     return false;
   }
@@ -41,6 +55,14 @@ bool TestServer::LaunchPython(const FilePath& testserver_path) {
   return true;
 }
 
+bool TestServer::WaitToStart() {
+  char buf[8];
+  ssize_t n = HANDLE_EINTR(read(child_fd_, buf, sizeof(buf)));
+  // We don't need the FD anymore.
+  child_fd_closer_.reset(NULL);
+  return n > 0;
+}
+
 bool TestServer::CheckCATrusted() {
   return true;
 }
diff --git a/net/test/test_server_win.cc b/net/test/test_server_win.cc
index 7e8443f..a8b3678 100644
--- a/net/test/test_server_win.cc
+++ b/net/test/test_server_win.cc
@@ -34,7 +34,7 @@ bool LaunchTestServerAsJob(const std::wstring& cmdline,
   // The CREATE_BREAKAWAY_FROM_JOB flag is used to prevent this.
   if (!CreateProcess(NULL,
                      const_cast<wchar_t*>(cmdline.c_str()), NULL, NULL,
-                     FALSE, CREATE_BREAKAWAY_FROM_JOB, NULL, NULL,
+                     TRUE, CREATE_BREAKAWAY_FROM_JOB, NULL, NULL,
                      &startup_info, &process_info)) {
     LOG(ERROR) << "Could not create process.";
     return false;
@@ -107,6 +107,36 @@ bool TestServer::LaunchPython(const FilePath& testserver_path) {
   if (type_ == TYPE_HTTPS_CLIENT_AUTH)
     command_line.append(L" --ssl-client-auth");
 
+  HANDLE child_read = NULL;
+  HANDLE child_write = NULL;
+  if (!CreatePipe(&child_read, &child_write, NULL, 0)) {
+    PLOG(ERROR) << "Failed to create pipe";
+    return false;
+  }
+  child_fd_.Set(child_read);
+  ScopedHandle scoped_child_write(child_write);
+
+  // Have the child inherit the write half.
+  if (!SetHandleInformation(child_write, HANDLE_FLAG_INHERIT,
+                            HANDLE_FLAG_INHERIT)) {
+    PLOG(ERROR) << "Failed to enable pipe inheritance";
+    return false;
+  }
+
+  // Pass the handle on the command-line. Although HANDLE is a
+  // pointer, truncating it on 64-bit machines is okay. See
+  // http://msdn.microsoft.com/en-us/library/aa384203.aspx
+  //
+  // "64-bit versions of Windows use 32-bit handles for
+  // interoperability. When sharing a handle between 32-bit and 64-bit
+  // applications, only the lower 32 bits are significant, so it is
+  // safe to truncate the handle (when passing it from 64-bit to
+  // 32-bit) or sign-extend the handle (when passing it from 32-bit to
+  // 64-bit)."
+  command_line.append(
+      L" --startup-pipe=" +
+      ASCIIToWide(base::IntToString(reinterpret_cast<uintptr_t>(child_write))));
+
   if (!LaunchTestServerAsJob(command_line,
                              true,
                              &process_handle_,
@@ -118,6 +148,14 @@ bool TestServer::LaunchPython(const FilePath& testserver_path) {
   return true;
 }
 
+bool TestServer::WaitToStart() {
+  char buf[8];
+  DWORD bytes_read;
+  BOOL result = ReadFile(child_fd_, buf, sizeof(buf), &bytes_read, NULL);
+  child_fd_.Close();
+  return result && bytes_read > 0;
+}
+
 bool TestServer::CheckCATrusted() {
   HCERTSTORE cert_store = CertOpenSystemStore(NULL, L"ROOT");
   if (!cert_store) {
diff --git a/net/tools/testserver/testserver.py b/net/tools/testserver/testserver.py
index 8a864a8..ee2476c 100644
--- a/net/tools/testserver/testserver.py
+++ b/net/tools/testserver/testserver.py
@@ -39,6 +39,9 @@ except ImportError:
   import md5
   _new_md5 = md5.new
 
+if sys.platform == 'win32':
+  import msvcrt
+
 SERVER_HTTP = 0
 SERVER_FTP = 1
 
@@ -1230,6 +1233,17 @@ def main(options, args):
     server = pyftpdlib.ftpserver.FTPServer(address, ftp_handler)
     print 'FTP server started on port %d...' % port
 
+  # Notify the parent that we've started. (BaseServer subclasses
+  # bind their sockets on construction.)
+  if options.startup_pipe is not None:
+    if sys.platform == 'win32':
+      fd = msvcrt.open_osfhandle(options.startup_pipe, 0)
+    else:
+      fd = options.startup_pipe
+    startup_pipe = os.fdopen(fd, "w")
+    startup_pipe.write("READY")
+    startup_pipe.close()
+
   try:
     server.serve_forever()
   except KeyboardInterrupt:
@@ -1263,6 +1277,9 @@ if __name__ == '__main__':
                            help='Prevent the server from dying when visiting '
                            'a /kill URL. Useful for manually running some '
                            'tests.')
+  option_parser.add_option('', '--startup-pipe', type='int',
+                           dest='startup_pipe',
+                           help='File handle of pipe to parent process')
   options, args = option_parser.parse_args()
 
   sys.exit(main(options, args))