Replace StreamListenSocket with StreamSocket in HttpServer.

net/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 NET_SERVER_HTTP_SERVER_H_
 #define NET_SERVER_HTTP_SERVER_H_
 
-#include <list>
 #include <map>

[mmenke, 2014/06/04 16:29:50]

need std::string.

[byungchul, 2014/06/06 19:17:35]

Done.

 
-#include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "net/http/http_status_code.h"
-#include "net/socket/stream_listen_socket.h"
+#include "net/server/http_connection.h"

[mmenke, 2014/06/04 16:29:50]

Can just forward declare this, I believe.

[byungchul, 2014/06/06 19:17:35]

Done.

 
 namespace net {
 
-class HttpConnection;
 class HttpServerRequestInfo;
 class HttpServerResponseInfo;
 class IPEndPoint;
+class ServerSocket;
+class StreamSocket;
 class WebSocket;
 
-class HttpServer : public StreamListenSocket::Delegate,
-                   public base::RefCountedThreadSafe<HttpServer> {
+class NET_EXPORT HttpServer {
  public:
   class Delegate {
    public:
     virtual void OnHttpRequest(int connection_id,
                                const HttpServerRequestInfo& info) = 0;
 
     virtual void OnWebSocketRequest(int connection_id,
                                     const HttpServerRequestInfo& info) = 0;
 
     virtual void OnWebSocketMessage(int connection_id,
                                     const std::string& data) = 0;
 
     virtual void OnClose(int connection_id) = 0;
-
-   protected:
-    virtual ~Delegate() {}
   };
 
-  HttpServer(const StreamListenSocketFactory& socket_factory,
+  HttpServer(scoped_ptr<ServerSocket> server_socket,
              HttpServer::Delegate* delegate);
+  ~HttpServer();
 
   void AcceptWebSocket(int connection_id,
                        const HttpServerRequestInfo& request);
   void SendOverWebSocket(int connection_id, const std::string& data);
   // Sends the provided data directly to the given connection. No validation is
   // performed that data constitutes a valid HTTP response. A valid HTTP
   // response may be split across multiple calls to SendRaw.
   void SendRaw(int connection_id, const std::string& data);
   void SendResponse(int connection_id, const HttpServerResponseInfo& response);
   void Send(int connection_id,
             HttpStatusCode status_code,
             const std::string& data,
             const std::string& mime_type);
   void Send200(int connection_id,
                const std::string& data,

[mmenke, 2014/06/04 16:29:50]

I still think we should really be taking IOBuffers here.  That would mean that
we'd need a second IOBuffer for just the headers, but that would allow fewer
copies, for smart consumers.

[byungchul, 2014/06/04 17:41:28]

By nature of IOBuffer which doesn't have size(), it is shared by caller and
callee. caller just hands the ownership of [IOBuffer->data(), IOBuffer->data() +
buf_size passed in function] portion over to the callee. Other portion is still
owned by the caller. From this nature, it should be used with callback for
callee to hand the ownership back to the caller.
So, we have options:
1) Use a specific IOBuffer, for example, StringIOBuffer which copies data once
already. I don't see any benefits in terms of performance.
2) Take the ownership for all portion of IOBuffer and never returns back to the
caller, where caller must create IOBuffer each time and copy data (again).
Otherwise, it is hard for callee to maintain the lifetime of internal data of
IOBuffer. I think this can lead confusions and errors. 
3) Change http server model to accept IOBuffer, size and callback as other
interfaces, where caller must implement buffering like one of this CL.

If IOBuffer is only for performace, I don't think we can gain much. If it is to
unify the api, can we do it in next CL?

[mmenke, 2014/06/04 19:27:50]

Currently, things are like this:  Embedder creates a string.  We copy the string
into HttpServerResponse info.  We then serialize the string and headers in a
single buffer.  We then copy them into the IOBuffer.  This is 3 extra copies,
none of which are necessary.  And when we're talking about 100MB (!) of data,
this just seems like a really bad idea.

If the embedder passed us an IOBuffer (And a length), then there would be no
extra copies, if the embedder created the data in the IOBuffer directly, or one
extra copy, if it created a string, and copied it to the IOBuffer.

Alternatively, we could keep the current API and just do a single copy, straight
into an IOBuffer, and put the headers in another buffer.

[byungchul, 2014/06/04 20:51:26]

The main purpose of this CL is replacing SteamListenSocket with ServerSocket.
Changing interface can be done later because this implementation doesn't add
copy more than before.
I would take your alternative way if you don't mind.

[mmenke, 2014/06/04 21:00:00]

Longer term, I'd love  to see it changed, but I'm happy with the alternative,
for now.

[byungchul, 2014/06/06 19:17:35]

Done.

                const std::string& mime_type);
   void Send404(int connection_id);
   void Send500(int connection_id, const std::string& message);
 
   void Close(int connection_id);
 
+  void SetReceiveBufferSize(int connection_id, int32 size);
+  void SetSendBufferSize(int connection_id, int32 size);
+
   // Copies the local address to |address|. Returns a network error code.
   int GetLocalAddress(IPEndPoint* address);
 
-  // ListenSocketDelegate
-  virtual void DidAccept(StreamListenSocket* server,
-                         scoped_ptr<StreamListenSocket> socket) OVERRIDE;
-  virtual void DidRead(StreamListenSocket* socket,
-                       const char* data,
-                       int len) OVERRIDE;
-  virtual void DidClose(StreamListenSocket* socket) OVERRIDE;
-
- protected:
-  virtual ~HttpServer();
+  base::WeakPtr<HttpServer> GetWeakPtr();

[mmenke, 2014/06/04 16:29:50]

It's generally a bad idea to expose weak pointers externally, unless it's an
expected part of the class's externally visible behavior.

If DevTools really needs one, it should create its own.

[byungchul, 2014/06/04 17:41:28]

DevTools posts tasks on http server. Do you mean devtools should post tasks on
itself and call http server's functions? Though it sounds inefficient, doable.
By the way, how can we determine whether or not "it's an
expected part of the class's externally visible behavior"?

[mmenke, 2014/06/04 19:27:50]

If the purpose of the class is related to message passing in a safe manner, it
might be a logical part of the API.  That's a pretty rare case, however.

There doesn't seem to be any reason net should have knowledge that the embedder
(DevTools) wants to use it with a WeakPtr.  In general, exposing WeakPtrs to
external classes is an anti-pattern that should be avoided.

[byungchul, 2014/06/06 19:17:35]

Removed. Now devtools destroys objects which have the reference of http server
before it destroys http server.

 
  private:
-  friend class base::RefCountedThreadSafe<HttpServer>;
-  friend class HttpConnection;
+  friend class HttpServerTest;
+
+  typedef std::map<int, HttpConnection*> IdToConnectionMap;
+
+  void DoAcceptLoop();
+  void OnAcceptCompleted(int rv);
+  int DidAccept(int rv);
+
+  void DoReadLoop(HttpConnection* connection);
+  void OnReadCompleted(int connection_id, int rv);
+  int DidRead(HttpConnection* conn, int rv);
+
+  void DoWriteLoop(HttpConnection* connection);
+  void OnWriteCompleted(int connection_id, int rv);
+  int DidWrite(HttpConnection* conn, int rv);
 
   // Expects the raw data to be stored in recv_data_. If parsing is successful,
   // will remove the data parsed from recv_data_, leaving only the unused
   // recv data.
-  bool ParseHeaders(HttpConnection* connection,
+  bool ParseHeaders(const char* data,
+                    size_t data_len,
                     HttpServerRequestInfo* info,
                     size_t* pos);
 
   HttpConnection* FindConnection(int connection_id);
-  HttpConnection* FindConnection(StreamListenSocket* socket);
 
-  HttpServer::Delegate* delegate_;
-  scoped_ptr<StreamListenSocket> server_;
-  typedef std::map<int, HttpConnection*> IdToConnectionMap;
+  const scoped_ptr<ServerSocket> server_socket_;
+  scoped_ptr<StreamSocket> accepted_socket_;
+  HttpServer::Delegate* const delegate_;
+
+  int last_id_;
   IdToConnectionMap id_to_connection_;
-  typedef std::map<StreamListenSocket*, HttpConnection*> SocketToConnectionMap;
-  SocketToConnectionMap socket_to_connection_;
+
+  base::WeakPtrFactory<HttpServer> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(HttpServer);

[mmenke, 2014/06/04 16:29:50]

Should include macros.h

[byungchul, 2014/06/06 19:17:35]

Done.

 };
 
 }  // namespace net
 
 #endif // NET_SERVER_HTTP_SERVER_H_