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)
@@ -9,6 +9,7 @@
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/compiler_specific.h"
+#include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/metrics/field_trial.h"
@@ -237,7 +238,7 @@
       is_secure_(false),
       certificate_error_code_(OK),
       error_(OK),
-      state_(IDLE),
+      state_(STATE_IDLE),
       max_concurrent_streams_(initial_max_concurrent_streams == 0 ?
                               kInitialMaxConcurrentStreams :
                               initial_max_concurrent_streams),
@@ -248,7 +249,8 @@
       streams_pushed_count_(0),
       streams_pushed_and_claimed_count_(0),
       streams_abandoned_count_(0),
-      bytes_received_(0),
+      total_bytes_received_(0),
+      bytes_read_(0),
       sent_settings_(false),
       received_settings_(false),
       stalled_streams_(0),
@@ -308,8 +310,8 @@
 SpdySession::CallbackResultPair::~CallbackResultPair() {}
 
 SpdySession::~SpdySession() {
-  if (state_ != CLOSED) {
-    state_ = CLOSED;
+  if (state_ != STATE_CLOSED) {
+    state_ = STATE_CLOSED;
 
     // Cleanup all the streams.
     CloseAllStreams(net::ERR_ABORTED);
@@ -338,7 +340,7 @@
   base::StatsCounter spdy_sessions("spdy.sessions");
   spdy_sessions.Increment();
 
-  state_ = CONNECTED;
+  state_ = STATE_DO_READ;
   connection_.reset(connection);
   is_secure_ = is_secure;
   certificate_error_code_ = certificate_error_code;
@@ -370,7 +372,7 @@
 
   // Write out any data that we might have to send, such as the settings frame.
   WriteSocketLater();
-  net::Error error = ReadSocket();
+  net::Error error = static_cast<net::Error>(DoLoop(OK));

[Ryan Hamilton, 2012/12/29 18:41:52]

nit:  int rv = DoLoop(OK);

[ramant (doing other things), 2013/01/16 00:31:58]

Done.

   if (error == ERR_IO_PENDING)
     return OK;
   return error;
@@ -380,7 +382,7 @@
   if (!verify_domain_authentication_)
     return true;
 
-  if (state_ != CONNECTED)
+  if (!IsConnected())
     return false;
 
   SSLInfo ssl_info;
@@ -408,7 +410,7 @@
     const GURL& url,
     scoped_refptr<SpdyStream>* stream,
     const BoundNetLog& stream_net_log) {
-  CHECK_NE(state_, CLOSED);
+  CHECK_NE(state_, STATE_CLOSED);
 
   *stream = NULL;
 
@@ -764,7 +766,7 @@
 
   // If we're connecting, defer to the connection to give us the actual
   // LoadState.
-  if (state_ == CONNECTING)
+  if (state_ == STATE_CONNECTING)
     return connection_->GetLoadState();
 
   // Just report that we're idle since the session could be doing
@@ -773,11 +775,75 @@
 }
 
 void SpdySession::OnReadComplete(int bytes_read) {
+  DCHECK_NE(state_, STATE_DO_READ);
+  read_pending_ = false;
+  DoLoop(bytes_read);
+}
+
+void SpdySession::StartRead() {
+  DCHECK_NE(state_, STATE_DO_READ_COMPLETE);
+  read_pending_ = false;
+  DoLoop(OK);
+}
+
+int SpdySession::DoLoop(int result) {
+  TRACE_EVENT0("spdy", "SpdySession::DoLoop");
+  bytes_read_ = 0;
+  do {
+    if (read_pending_)
+      return OK;
+
+    switch (state_) {
+      case STATE_DO_READ:
+        DCHECK_EQ(result, OK);
+        result = DoRead();
+        break;
+      case STATE_DO_READ_COMPLETE:
+        result = DoReadComplete(result);
+        break;
+      case STATE_CLOSED:
+        result = ERR_CONNECTION_CLOSED;
+        break;
+      default:
+        NOTREACHED() << "state_: " << state_;
+        break;
+    }
+  } while (result != ERR_IO_PENDING && result != ERR_CONNECTION_CLOSED);
+
+  if (result == net::ERR_IO_PENDING)
+    read_pending_ = true;

[Ryan Hamilton, 2012/12/29 18:41:52]

nit: I would move these two lines to DoRead.

[ramant (doing other things), 2013/01/16 00:31:58]

Done.

+  return result;
+}
+
+int SpdySession::DoRead() {
+  TRACE_EVENT0("spdy", "SpdySession::DoRead");
+  const int kMaxReadBytes = 32 * 1024;
+  if (bytes_read_ > kMaxReadBytes) {

[Ryan Hamilton, 2012/12/29 18:41:52]

bytes_read_ is currently being set to 0 in DoLoop().  How does that interact
with this expression?

[ramant (doing other things), 2013/01/16 00:31:58]

We increment bytes_read_ in DoReadComplete. It is initialized to 0, before we
start reading and we read continuously 32k bytes without yielding and then we
yield by calling DoLoop (from StartRead) and we start reading continuously for
another 32k bytes.

+    state_ = STATE_DO_READ;
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&SpdySession::StartRead,
+                   weak_factory_.GetWeakPtr()));
+    return ERR_IO_PENDING;
+  }
+
+  DCHECK(!read_pending_);

[Ryan Hamilton, 2012/12/29 18:41:52]

nit Should this be moved above the if()?

[ramant (doing other things), 2013/01/16 00:31:58]

Done.

+  CHECK(connection_.get());
+  CHECK(connection_->socket());
+  state_ = STATE_DO_READ_COMPLETE;
+  return connection_->socket()->Read(
+      read_buffer_.get(),
+      kReadBufferSize,
+      base::Bind(&SpdySession::OnReadComplete, base::Unretained(this)));
+}
+
+int SpdySession::DoReadComplete(int bytes_read) {
   // Parse a frame.  For now this code requires that the frame fit into our
   // buffer (32KB).
   // TODO(mbelshe): support arbitrarily large frames!
 
-  read_pending_ = false;
+  TRACE_EVENT0("spdy", "SpdySession::DoReadComplete");
+  DCHECK(!read_pending_);
 
   if (bytes_read <= 0) {
     // Session is tearing down.
@@ -785,10 +851,11 @@
     if (bytes_read == 0)
       error = ERR_CONNECTION_CLOSED;
     CloseSessionOnError(error, true, "bytes_read is <= 0.");
-    return;
+    return ERR_CONNECTION_CLOSED;

[Ryan Hamilton, 2012/12/29 18:41:52]

Shouldn't this say "return error;"?

[ramant (doing other things), 2013/01/16 00:31:58]

CloseSessionOnError is logging the error. Thought it is better to return what is
current state of connection. 

If state is ERR_CONNECTION_CLOSED, higher level code knows the socket is closed
and it doesn't perform extra reads (for example during tear down of tests).

Done. 

   }
 
-  bytes_received_ += bytes_read;
+  total_bytes_received_ += bytes_read;
+  bytes_read_ += bytes_read;

[Ryan Hamilton, 2012/12/29 18:41:52]

I'm confused about the difference between bytes_read_ and total_bytes_read_. 
Can you add a comment?

[ramant (doing other things), 2013/01/16 00:31:58]

Added a comment in the header file.

Done.

 
   last_activity_time_ = base::TimeTicks::Now();
 
@@ -811,8 +878,9 @@
       buffered_spdy_framer_->Reset();
   }
 
-  if (state_ != CLOSED)
-    ReadSocket();
+  if (state_ != STATE_CLOSED)
+    state_ = STATE_DO_READ;
+  return OK;
 }
 
 void SpdySession::OnWriteComplete(int result) {
@@ -867,49 +935,11 @@
   }
 }
 
-net::Error SpdySession::ReadSocket() {
-  if (read_pending_)
-    return OK;
-
-  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;
-}
-
 void SpdySession::WriteSocketLater() {
   if (delayed_write_pending_)
     return;
 
-  if (state_ < CONNECTED)
+  if (!IsConnected())
     return;
 
   delayed_write_pending_ = true;
@@ -926,7 +956,7 @@
   // If the socket isn't connected yet, just wait; we'll get called
   // again when the socket connection completes.  If the socket is
   // closed, just return.
-  if (state_ < CONNECTED || state_ == CLOSED)
+  if (!IsConnected())
     return;
 
   if (write_pending_)   // Another write is in progress still.
@@ -1048,8 +1078,8 @@
   // Don't close twice.  This can occur because we can have both
   // a read and a write outstanding, and each can complete with
   // an error.
-  if (state_ != CLOSED) {
-    state_ = CLOSED;
+  if (state_ != STATE_CLOSED) {

[Ryan Hamilton, 2012/12/29 18:41:52]

if (!IsConnected()) {

[ramant (doing other things), 2013/01/16 00:31:58]

Changed it to !IsClosed() (in case connect failed).
Done.

+    state_ = STATE_CLOSED;
     error_ = err;
     if (remove_from_pool)
       RemoveFromPool();
@@ -1908,16 +1938,16 @@
           // for larger volumes of data being sent.
           UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsCwnd",
                                       val, 1, 200, 100);
-          if (bytes_received_ > 10 * 1024) {
+          if (total_bytes_received_ > 10 * 1024) {
             UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsCwnd10K",
                                         val, 1, 200, 100);
-            if (bytes_received_ > 25 * 1024) {
+            if (total_bytes_received_ > 25 * 1024) {
               UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsCwnd25K",
                                           val, 1, 200, 100);
-              if (bytes_received_ > 50 * 1024) {
+              if (total_bytes_received_ > 50 * 1024) {
                 UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsCwnd50K",
                                             val, 1, 200, 100);
-                if (bytes_received_ > 100 * 1024) {
+                if (total_bytes_received_ > 100 * 1024) {
                   UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsCwnd100K",
                                               val, 1, 200, 100);
                 }