SPDY - implement greedy approach to read all the data and process it

net/spdy/spdy_session.cc

Index: net/spdy/spdy_session.cc
===================================================================
--- net/spdy/spdy_session.cc	(revision 174489)
+++ net/spdy/spdy_session.cc	(working copy)
@@ -779,13 +779,20 @@
 
   read_pending_ = false;
 
+  bool can_do_more = ProcessBytesRead(bytes_read);
+
+  if (can_do_more)
+    ReadSocket();
+}
+
+bool SpdySession::ProcessBytesRead(int bytes_read) {
   if (bytes_read <= 0) {
     // Session is tearing down.
     net::Error error = static_cast<net::Error>(bytes_read);
     if (bytes_read == 0)
       error = ERR_CONNECTION_CLOSED;
     CloseSessionOnError(error, true, "bytes_read is <= 0.");
-    return;
+    return false;
   }
 
   bytes_received_ += bytes_read;
@@ -810,9 +817,7 @@
     if (buffered_spdy_framer_->state() == SpdyFramer::SPDY_DONE)
       buffered_spdy_framer_->Reset();
   }
-
-  if (state_ != CLOSED)
-    ReadSocket();
+  return state_ != CLOSED;
 }
 
 void SpdySession::OnWriteComplete(int result) {
@@ -868,41 +873,41 @@
 }
 
 net::Error SpdySession::ReadSocket() {
-  if (read_pending_)
-    return OK;
+  bool can_do_more = false;
+  net::Error result;
+  do {
+    if (read_pending_)
+      return OK;
 
-  if (state_ == CLOSED) {
-    NOTREACHED();
-    return ERR_UNEXPECTED;
-  }
+    if (state_ == CLOSED) {
+      NOTREACHED();
+      return ERR_UNEXPECTED;
+    }
 
-  CHECK(connection_.get());
-  CHECK(connection_->socket());
-  int bytes_read = connection_->socket()->Read(
-      read_buffer_.get(),
-      kReadBufferSize,
-      base::Bind(&SpdySession::OnReadComplete, base::Unretained(this)));
-  switch (bytes_read) {
-    case 0:
-      // Socket is closed!
-      CloseSessionOnError(ERR_CONNECTION_CLOSED, true, "bytes_read is 0.");
-      return ERR_CONNECTION_CLOSED;
-    case net::ERR_IO_PENDING:
-      // Waiting for data.  Nothing to do now.
-      read_pending_ = true;
-      return ERR_IO_PENDING;
-    default:
-      // Data was read, process it.
-      // Schedule the work through the message loop to avoid recursive
-      // callbacks.
-      read_pending_ = true;
-      MessageLoop::current()->PostTask(
-          FROM_HERE,
-          base::Bind(&SpdySession::OnReadComplete,
-                     weak_factory_.GetWeakPtr(), bytes_read));
-      break;
-  }
-  return OK;
+    CHECK(connection_.get());
+    CHECK(connection_->socket());
+    int bytes_read = connection_->socket()->Read(
+        read_buffer_.get(),

[willchan no longer on Chromium, 2012/12/24 00:19:35]

don't we need to increment the offset we read into this buffer?

[Ryan Sleevi, 2012/12/24 05:38:44]

No, because ProcessBytesRead drains |read_buffer_|

+        kReadBufferSize,
+        base::Bind(&SpdySession::OnReadComplete, base::Unretained(this)));
+    switch (bytes_read) {
+      case 0:
+        // Socket is closed!
+        CloseSessionOnError(ERR_CONNECTION_CLOSED, true, "bytes_read is 0.");
+        result = ERR_CONNECTION_CLOSED;
+        break;
+      case net::ERR_IO_PENDING:
+        // Waiting for data.  Nothing to do now.
+        read_pending_ = true;
+        result = ERR_IO_PENDING;
+        break;
+      default:
+        can_do_more = ProcessBytesRead(bytes_read);
+        result = OK;
+        break;
+    }
+  } while (result == OK && can_do_more);

[Ryan Sleevi, 2012/12/24 05:38:44]

Like I expressed Friday, I'm a slightly concerned with this pattern, if only
because it's significantly different from how we normally handle reads and the
state machine.

In particular, the way this is currently implemented requires duplicating the
error handling logic (lines 894-903 and lines 789-795), splitting the handling
of values < 0 from line 905 (eg: not handled) and line 789 (invoked by the
callback from line 782)

It does seem like putting in an explicit state machine logic to flip between
these states makes it clearer to the reader the flow, and lets you keep error
handling consistently in the same place. It will also make the concept map
between (TCPClientSocket, SSLClientSocket, SpdyStream, ProxyService,
QuicHttpStream,  etc) and this code.

[Ryan Hamilton, 2012/12/24 15:26:35]

+1 on an explict STATE_DO_READ/STATE_DO_READ_COMPLETE to remove the duplication.

+  return result;
 }
 
 void SpdySession::WriteSocketLater() {