Break dependencies preventing move of test_server down to net.

chrome/browser/google_apis/test_server/http_server.h

 // Copyright (c) 2012 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 CHROME_BROWSER_GOOGLE_APIS_TEST_SERVER_HTTP_SERVER_H_
 #define CHROME_BROWSER_GOOGLE_APIS_TEST_SERVER_HTTP_SERVER_H_
 
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #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 {
 
 class HttpConnection;
 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);
   virtual void Listen();
 
  private:
   virtual ~HttpListenSocket();
 };
 
 // Class providing an HTTP server for testing purpose. This is a basic server
@@ skipping 17 matching lines @@
 //   if (absolute_url.path() != "/test")
 //     return scoped_ptr<HttpResponse>();
 //
 //   scoped_ptr<HttpResponse> http_response(new HttpResponse());
 //   http_response->set_code(test_server::SUCCESS);
 //   http_response->set_content("hello");
 //   http_response->set_content_type("text/plain");
 //   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 {
     return listen_socket_.get() != NULL;
   }
 
   // Returns the base URL to the server, which looks like
   // http://127.0.0.1:<port>/, where <port> is the actual port number used by
   // the server.
   const GURL& base_url() const { return base_url_; }
 
   // Returns a URL to the server based on the given relative URL, which
   // should start with '/'. For example: GetURL("/path?query=foo") =>
   // http://127.0.0.1:<port>/path?query=foo.
   GURL GetURL(const std::string& relative_url) const;
 
   // Returns the port number used by the server.
   int port() const { return port_; }
 
   // The most general purpose method. Any request processing can be added using
   // this method. Takes ownership of the object. The |callback| is called
   // on UI thread.
   void RegisterRequestHandler(const HandleRequestCallback& callback);
 
  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.
   void HandleRequest(HttpConnection* connection,
                      scoped_ptr<HttpRequest> request);
 
   // net::StreamListenSocket::Delegate overrides:
   virtual void DidAccept(net::StreamListenSocket* server,
                          net::StreamListenSocket* connection) OVERRIDE;
   virtual void DidRead(net::StreamListenSocket* connection,
                        const char* data,
                        int length) OVERRIDE;
   virtual void DidClose(net::StreamListenSocket* connection) OVERRIDE;
 
   HttpConnection* FindConnection(net::StreamListenSocket* socket);
 
+  scoped_refptr<base::SingleThreadTaskRunner> io_thread_;
+
   scoped_refptr<HttpListenSocket> listen_socket_;
   int port_;
   GURL base_url_;
 
   // Owns the HttpConnection objects.
   std::map<net::StreamListenSocket*, HttpConnection*> connections_;
 
   // Vector of registered request handlers.
   std::vector<HandleRequestCallback> request_handlers_;
 
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.
   base::WeakPtrFactory<HttpServer> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(HttpServer);
 };
 
 }  // namespace test_servers
 }  // namespace google_apis
 
 #endif  // CHROME_BROWSER_GOOGLE_APIS_TEST_SERVER_HTTP_SERVER_H_