Replace StreamListenSocket with StreamSocket in HttpServer.

net/server/http_server.cc

Index: net/server/http_server.cc
diff --git a/net/server/http_server.cc b/net/server/http_server.cc
index f746f066e421798b681ac362b8767682d7883a63..e51f84bce8e2cee36f0b5c2d053d76ac7bf0eec1 100644
--- a/net/server/http_server.cc
+++ b/net/server/http_server.cc
@@ -17,14 +17,33 @@
 #include "net/server/http_server_request_info.h"
 #include "net/server/http_server_response_info.h"
 #include "net/server/web_socket.h"
-#include "net/socket/tcp_listen_socket.h"
+#include "net/socket/server_socket.h"
+#include "net/socket/stream_socket.h"
+#include "net/socket/tcp_server_socket.h"
 
 namespace net {
 
-HttpServer::HttpServer(const StreamListenSocketFactory& factory,
+namespace {
+
+// A helper function to delete connection in next run loop.
+void DeleteConnection(HttpConnection* connection) {
+  delete connection;
+}
+
+}  // namespace
+
+HttpServer::HttpServer(scoped_ptr<ServerSocket> server_socket,
                        HttpServer::Delegate* delegate)
-    : delegate_(delegate),
-      server_(factory.CreateAndListen(this)) {
+    : server_socket_(server_socket.Pass()),
+      delegate_(delegate),
+      last_id_(0) {
+  DCHECK(server_socket_);
+  DoAcceptLoop();
+}
+
+HttpServer::~HttpServer() {
+  STLDeleteContainerPairSecondPointers(
+      id_to_connection_.begin(), id_to_connection_.end());
 }
 
 void HttpServer::AcceptWebSocket(
@@ -33,9 +52,8 @@ void HttpServer::AcceptWebSocket(
   HttpConnection* connection = FindConnection(connection_id);
   if (connection == NULL)
     return;
-
-  DCHECK(connection->web_socket_.get());
-  connection->web_socket_->Accept(request);
+  DCHECK(connection->web_socket());
+  connection->web_socket()->Accept(request);
 }
 
 void HttpServer::SendOverWebSocket(int connection_id,
@@ -43,23 +61,23 @@ void HttpServer::SendOverWebSocket(int connection_id,
   HttpConnection* connection = FindConnection(connection_id);
   if (connection == NULL)
     return;
-  DCHECK(connection->web_socket_.get());
-  connection->web_socket_->Send(data);
+  DCHECK(connection->web_socket());
+  connection->web_socket()->Send(data);
 }
 
 void HttpServer::SendRaw(int connection_id, const std::string& data) {
   HttpConnection* connection = FindConnection(connection_id);
   if (connection == NULL)
     return;
-  connection->Send(data);
+
+  bool writing_in_progress = !connection->write_buf()->IsEmpty();
+  if (connection->write_buf()->Append(data) && !writing_in_progress)
+    DoWriteLoop(connection);
 }
 
 void HttpServer::SendResponse(int connection_id,
                               const HttpServerResponseInfo& response) {

[mmenke, 2014/07/24 20:50:08]

Maybe rename this to SendResponseHeaders?  Could optioanlly also rename
HttpServerResponseInfo to HttpServerResponseHeaders

[byungchul, 2014/08/05 07:06:19]

It's not just for headers. HttpServerResponseInfo has data. Added a TODO to
consider function name change though.

-  HttpConnection* connection = FindConnection(connection_id);
-  if (connection == NULL)
-    return;
-  connection->Send(response);
+  SendRaw(connection_id, response.Serialize());
 }
 
 void HttpServer::Send(int connection_id,
@@ -67,8 +85,9 @@ void HttpServer::Send(int connection_id,
                       const std::string& data,
                       const std::string& content_type) {
   HttpServerResponseInfo response(status_code);
-  response.SetBody(data, content_type);
+  response.SetContentHeaders(data.size(), content_type);
   SendResponse(connection_id, response);
+  SendRaw(connection_id, data);
 }
 
 void HttpServer::Send200(int connection_id,
@@ -90,38 +109,110 @@ void HttpServer::Close(int connection_id) {
   if (connection == NULL)
     return;
 
-  // Initiating close from server-side does not lead to the DidClose call.
-  // Do it manually here.
-  DidClose(connection->socket_.get());
+  id_to_connection_.erase(connection->id());
+  delegate_->OnClose(connection_id);
+
+  // The call stack might have callbacks which still have the pointer of
+  // connection. Instead of referencing connection with ID all the time,
+  // destroys the connection in next run loop to make sure any pending
+  // callbacks in the call stack return.

[mmenke, 2014/07/24 20:50:09]

What callbacks?  SocketLibEvent can handle being deleted while calling a
callback, no?

[byungchul, 2014/08/05 07:06:19]

This is a server. So, it is a common pattern to do something on read operation
which may close the connection. It is not only for close on error. This logic is
for that normal close by server side.

+  base::MessageLoopProxy::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&DeleteConnection, base::Unretained(connection)));

[mmenke, 2014/07/24 20:50:06]

DeleteHelper, maybe?

Both leak if the task is never run, during shutdown, but don't think we can fix
that - if we use base::Owned instead, then if this is called during shutdown,
the task will be destroyed instantly.

[byungchul, 2014/08/05 07:06:19]

Don't know how to use DeleteHelper correctly. Instead, changed to call
DeleteSoon.

 }
 
 int HttpServer::GetLocalAddress(IPEndPoint* address) {
-  if (!server_)
-    return ERR_SOCKET_NOT_CONNECTED;
-  return server_->GetLocalAddress(address);
+  return server_socket_->GetLocalAddress(address);
 }
 
-void HttpServer::DidAccept(StreamListenSocket* server,
-                           scoped_ptr<StreamListenSocket> socket) {
-  HttpConnection* connection = new HttpConnection(this, socket.Pass());
-  id_to_connection_[connection->id()] = connection;
-  // TODO(szym): Fix socket access. Make HttpConnection the Delegate.
-  socket_to_connection_[connection->socket_.get()] = connection;
+void HttpServer::SetReceiveBufferSize(int connection_id, int32 size) {
+  HttpConnection* connection = FindConnection(connection_id);
+  if (connection == NULL)
+    return;

[mmenke, 2014/07/24 20:50:06]

Is there a real need to let this be called for non-existent sockets?  Generally,
the preference is to crash if something should not happen.  Same goes for
SetSendBufferSize.

[byungchul, 2014/08/05 07:06:19]

Done.

+  connection->read_buf()->set_capacity_limit(size);
 }
 
-void HttpServer::DidRead(StreamListenSocket* socket,
-                         const char* data,
-                         int len) {
-  HttpConnection* connection = FindConnection(socket);
-  DCHECK(connection != NULL);
+void HttpServer::SetSendBufferSize(int connection_id, int32 size) {
+  HttpConnection* connection = FindConnection(connection_id);
   if (connection == NULL)
     return;
+  connection->write_buf()->set_total_size_limit(size);
+}
+
+void HttpServer::DoAcceptLoop() {
+  int rv;
+  do {
+    rv = server_socket_->Accept(&accepted_socket_,
+                                base::Bind(&HttpServer::OnAcceptCompleted,
+                                           base::Unretained(this)));
+    if (rv == ERR_IO_PENDING)
+      return;
+    rv = DidAccept(rv);
+  } while (rv == OK);
+}
+
+void HttpServer::OnAcceptCompleted(int rv) {
+  if (DidAccept(rv) == OK)
+    DoAcceptLoop();
+}
+
+int HttpServer::DidAccept(int rv) {
+  if (rv < 0) {
+    LOG(ERROR) << "Accept error: rv=" << rv;
+    return rv;
+  }
+
+  HttpConnection* connection =
+      new HttpConnection(++last_id_, accepted_socket_.Pass());
+  id_to_connection_[connection->id()] = connection;
+  DoReadLoop(connection);

[mmenke, 2014/07/24 20:50:09]

Random comment, feel free to ignore:  Hrm...we normally allow destruction of
HttpServers on close in DidRead.  However, we don't allow deletion of the
HttpServer in DidAccept, even when it calls DidRead.  So if we establish a
connection, try to read from it, synchronously get an error, return the error to
the Delegate, which then deletes us, looks like we crash.

I think the API really needs documentation about when it is and is not safe to
delete this.

[byungchul, 2014/08/05 07:06:19]

Even onClose() is not safe to destroy http server. Added a comment that in .h
file.

+  return OK;
+}
+
+void HttpServer::DoReadLoop(HttpConnection* connection) {
+  int rv;
+  do {
+    HttpConnection::ReadIOBuffer* read_buf = connection->read_buf();
+    // Increases read buffer size if necessary.
+    if (read_buf->GetUnusedCapacity() == 0 && !read_buf->IncreaseCapacity()) {
+      Close(connection->id());
+      return;
+    }
 
-  connection->recv_data_.append(data, len);
-  while (connection->recv_data_.length()) {
-    if (connection->web_socket_.get()) {
+    rv = connection->socket()->Read(
+        read_buf->GetUnusedIOBuffer(),
+        read_buf->GetUnusedCapacity(),
+        base::Bind(&HttpServer::OnReadCompleted,
+                   base::Unretained(this), connection->id()));
+    if (rv == ERR_IO_PENDING)
+      return;
+    rv = DidRead(connection, rv);
+  } while (rv == OK);
+}
+
+void HttpServer::OnReadCompleted(int connection_id, int rv) {
+  HttpConnection* connection = FindConnection(connection_id);
+  if (!connection)  // It might be closed right before by write error.

[mmenke, 2014/07/24 20:50:06]

What's to prevent the HttpServer from being destroyed during the period while
the connection still has a pointer to this, and the delete task is pending?

[byungchul, 2014/08/05 07:06:19]

connection doesn't have the reference for http server. And, added a comment that
any of delegate functions are not safe to destroy http server.

+    return;
+
+  if (DidRead(connection, rv) == OK)
+    DoReadLoop(connection);
+}
+
+int HttpServer::DidRead(HttpConnection* connection, int rv) {

[mmenke, 2014/07/24 20:50:07]

optional:  Suggest renaming this to HandleReadResult.  Think that makes it a
little clearer what's different between this and OnReadCompleted than the
current name.

Same suggestion for DidWrite.

[byungchul, 2014/08/05 07:06:19]

Done.

+  if (rv <= 0) {
+    Close(connection->id());
+    return rv == 0 ? ERR_CONNECTION_CLOSED : rv;
+  }
+
+  HttpConnection::ReadIOBuffer* read_buf = connection->read_buf();
+  read_buf->DidRead(rv);
+
+  // Handles http requests or websocket messages.
+  while (read_buf->GetUnconsumedSize() > 0) {
+    if (connection->web_socket()) {
       std::string message;
-      WebSocket::ParseResult result = connection->web_socket_->Read(&message);
+      WebSocket::ParseResult result = connection->web_socket()->Read(&message);
       if (result == WebSocket::FRAME_INCOMPLETE)
         break;
 
@@ -136,62 +227,88 @@ void HttpServer::DidRead(StreamListenSocket* socket,
 
     HttpServerRequestInfo request;
     size_t pos = 0;
-    if (!ParseHeaders(connection, &request, &pos))
+    if (!ParseHeaders(read_buf->data(), read_buf->GetUnconsumedSize(),
+                      &request, &pos))
       break;
 
     // Sets peer address if exists.
-    socket->GetPeerAddress(&request.peer);
+    connection->socket()->GetPeerAddress(&request.peer);
 
     if (request.HasHeaderValue("connection", "upgrade")) {
-      connection->web_socket_.reset(WebSocket::CreateWebSocket(connection,
-                                                               request,
-                                                               &pos));
-
-      if (!connection->web_socket_.get())  // Not enough data was received.
+      scoped_ptr<WebSocket> websocket(
+          WebSocket::CreateWebSocket(this, connection, request, &pos));
+      if (!websocket)  // Not enough data was received.
         break;
+      connection->SetWebSocket(websocket.Pass());
+      read_buf->DidConsume(pos);
       delegate_->OnWebSocketRequest(connection->id(), request);
-      connection->Shift(pos);
       continue;
     }
 
     const char kContentLength[] = "content-length";
-    if (request.headers.count(kContentLength)) {
+    if (request.headers.count(kContentLength) > 0) {
       size_t content_length = 0;
       const size_t kMaxBodySize = 100 << 20;
       if (!base::StringToSizeT(request.GetHeaderValue(kContentLength),
                                &content_length) ||
           content_length > kMaxBodySize) {
-        connection->Send(HttpServerResponseInfo::CreateFor500(
-            "request content-length too big or unknown: " +
-            request.GetHeaderValue(kContentLength)));
-        DidClose(socket);
+        SendResponse(connection->id(),
+                     HttpServerResponseInfo::CreateFor500(
+                         "request content-length too big or unknown: " +
+                         request.GetHeaderValue(kContentLength)));
+        Close(connection->id());
         break;
       }
 
-      if (connection->recv_data_.length() - pos < content_length)
+      if (read_buf->GetUnconsumedSize() - pos < content_length)
         break;  // Not enough data was received yet.
-      request.data = connection->recv_data_.substr(pos, content_length);
+      request.data.assign(read_buf->data() + pos, content_length);
       pos += content_length;
     }
 
+    read_buf->DidConsume(pos);
     delegate_->OnHttpRequest(connection->id(), request);
-    connection->Shift(pos);
   }
+
+  return OK;
 }
 
-void HttpServer::DidClose(StreamListenSocket* socket) {
-  HttpConnection* connection = FindConnection(socket);
-  DCHECK(connection != NULL);
-  id_to_connection_.erase(connection->id());
-  socket_to_connection_.erase(connection->socket_.get());
-  delete connection;
+void HttpServer::DoWriteLoop(HttpConnection* connection) {
+  int rv = OK;
+  HttpConnection::PendingWriteIOBuffer* write_buf = connection->write_buf();
+  while (rv == OK && write_buf->GetSizeToWrite() > 0) {
+    rv = connection->socket()->Write(
+        write_buf,
+        write_buf->GetSizeToWrite(),
+        base::Bind(&HttpServer::OnWriteCompleted,
+                   base::Unretained(this), connection->id()));
+    if (rv == ERR_IO_PENDING || rv == OK)
+      return;
+    rv = DidWrite(connection, rv);
+  }
 }
 
-HttpServer::~HttpServer() {
-  STLDeleteContainerPairSecondPointers(
-      id_to_connection_.begin(), id_to_connection_.end());
+void HttpServer::OnWriteCompleted(int connection_id, int rv) {
+  HttpConnection* connection = FindConnection(connection_id);
+  if (!connection)  // It might be closed right before by read error.
+    return;
+
+  if (DidWrite(connection, rv) == OK)
+    DoWriteLoop(connection);
+}
+
+int HttpServer::DidWrite(HttpConnection* connection, int rv) {
+  if (rv < 0) {
+    Close(connection->id());
+    return rv;
+  }
+
+  connection->write_buf()->DidConsume(rv);
+  return OK;
 }
 
+namespace {
+
 //
 // HTTP Request Parser
 // This HTTP request parser uses a simple state machine to quickly parse
@@ -255,17 +372,19 @@ int charToInput(char ch) {
   return INPUT_DEFAULT;
 }
 
-bool HttpServer::ParseHeaders(HttpConnection* connection,
+}  // namespace
+
+bool HttpServer::ParseHeaders(const char* data,
+                              size_t data_len,
                               HttpServerRequestInfo* info,
                               size_t* ppos) {
   size_t& pos = *ppos;
-  size_t data_len = connection->recv_data_.length();
   int state = ST_METHOD;
   std::string buffer;
   std::string header_name;
   std::string header_value;
   while (pos < data_len) {
-    char ch = connection->recv_data_[pos++];
+    char ch = data[pos++];
     int input = charToInput(ch);
     int next_state = parser_state[state][input];
 
@@ -337,11 +456,4 @@ HttpConnection* HttpServer::FindConnection(int connection_id) {
   return it->second;
 }
 
-HttpConnection* HttpServer::FindConnection(StreamListenSocket* socket) {
-  SocketToConnectionMap::iterator it = socket_to_connection_.find(socket);
-  if (it == socket_to_connection_.end())
-    return NULL;
-  return it->second;
-}
-
 }  // namespace net