Break dependencies preventing move of test_server down to net.

chrome/browser/google_apis/test_server/http_server.h

Index: chrome/browser/google_apis/test_server/http_server.h
diff --git a/chrome/browser/google_apis/test_server/http_server.h b/chrome/browser/google_apis/test_server/http_server.h
index 9f288eeb8c82bc16998bb6d6503a612a6fb99c56..fd4e8068f7ebc143d55069a63c6152c399821455 100644
--- a/chrome/browser/google_apis/test_server/http_server.h
+++ b/chrome/browser/google_apis/test_server/http_server.h
@@ -13,9 +13,14 @@
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/threading/non_thread_safe.h"
 #include "googleurl/src/gurl.h"
 #include "net/socket/tcp_listen_socket.h"
 
+namespace base {
+class WaitableEvent;
+};
+
 namespace google_apis {
 namespace test_server {
 
@@ -24,7 +29,8 @@ class HttpResponse;
 struct HttpRequest;
 
 // This class is required to be able to have composition instead of inheritance,
-class HttpListenSocket: public net::TCPListenSocket {
+class HttpListenSocket : public net::TCPListenSocket,
+                         public base::NonThreadSafe {

[satorux1, 2013/04/22 01:12:01]

In favor of less inheritance, please use base::ThreadWatcher.

non_thread_safe.h says:


// Note that base::ThreadChecker offers identical functionality to              
 
// NonThreadSafe, but does not require inheritence. In general, it is preferable
 
// to have a base::ThreadChecker as a member, rather than inherit from          
 
// NonThreadSafe. For more details about when to choose one over the other, see 
 
// the documentation for base::ThreadChecker.                                   
 

[Paweł Hajdan Jr., 2013/04/22 18:53:44]

Done.

  public:
   HttpListenSocket(const SocketDescriptor socket_descriptor,
                    net::StreamListenSocket::Delegate* delegate);
@@ -62,21 +68,23 @@ class HttpListenSocket: public net::TCPListenSocket {
 //   return http_response.Pass();
 // }
 //
-class HttpServer : public net::StreamListenSocket::Delegate {
+class HttpServer : public net::StreamListenSocket::Delegate,
+                   public base::NonThreadSafe {
  public:
   typedef base::Callback<scoped_ptr<HttpResponse>(const HttpRequest& request)>
       HandleRequestCallback;
 
   // Creates a http test server. InitializeAndWaitUntilReady() must be called
   // to start the server.
-  HttpServer();
+  explicit HttpServer(
+      const scoped_refptr<base::SingleThreadTaskRunner>& io_thread);
   virtual ~HttpServer();
 
   // Initializes and waits until the server is ready to accept requests.
-  bool InitializeAndWaitUntilReady();
+  bool InitializeAndWaitUntilReady() WARN_UNUSED_RESULT;
 
   // Shuts down the http server and waits until the shutdown is complete.
-  void ShutdownAndWaitUntilComplete();
+  bool ShutdownAndWaitUntilComplete() WARN_UNUSED_RESULT;
 
   // Checks if the server is started.
   bool Started() const {
@@ -104,10 +112,10 @@ class HttpServer : public net::StreamListenSocket::Delegate {
  private:
   // Initializes and starts the server. If initialization succeeds, Starts()
   // will return true.
-  void InitializeOnIOThread();
+  void InitializeOnIOThread(base::WaitableEvent* event);

[satorux1, 2013/04/22 01:12:01]

Could you avoid using WaitableEvent? My experience with WaitableEvent was horrid
so I'd like not to see it in this code.

[Paweł Hajdan Jr., 2013/04/22 18:53:44]

It'd be great to have a constructive alternative.

WaitableEvent is the best tool I know for this job, i.e. blocking execution of
the method until the other thread unblocks it.

Note that IMHO it's not OK to "dislike" base components in that way. My
experience was good, it's in base, reviewed and maintained.

Please provide what would be an alternative acceptable for you. I think taking
this as-is would also be fine.

[satorux1, 2013/04/23 01:31:04]

Sorry I wasn't clear. I'm not disliking WaitableEvent. What I concern about is
the complexity added by use of WaitableEvent. Synchronization between threads is
tricky. WaitableEvent is useful for that, but it'd be nicer if can avoid doing
that from the beginning (I spent quite some time getting rid of WaitableEvent
from some code in Chrome to fix all the obscure issues introduced by
WaitableEvent, hence my experience was bad).

The original code was solving this problem by
content::RunAllPendingInMessageLoop(), which was simpler than WaitableEvent
IMHO.


If there is no better alternative (I don't have a good idea at this moment),
WaitableEvent may be the way to go, but just wanted to raise a concern.

[satorux1, 2013/04/23 08:08:50]

FWIW, just to make it clearer:

issues introduced by WaitableEvent => issues introduced by *undesirable* use of
WaitableEvent.

 
   // Shuts down the server.
-  void ShutdownOnIOThread();
+  void ShutdownOnIOThread(base::WaitableEvent* event);
 
   // Handles a request when it is parsed. It passes the request to registed
   // request handlers and sends a http response.
@@ -124,6 +132,8 @@ class HttpServer : public net::StreamListenSocket::Delegate {
 
   HttpConnection* FindConnection(net::StreamListenSocket* socket);
 
+  scoped_refptr<base::SingleThreadTaskRunner> io_thread_;
+
   scoped_refptr<HttpListenSocket> listen_socket_;
   int port_;
   GURL base_url_;