Fix FlipSession cleanup to be unified through CloseSession. Because Flip is

full duplex, a closed socket will cause both read and write completions.  Unifying
the cleanup code allows both codepaths to safely cleanup the socket.

In the process, consolidated several flags on the session into a single State
variable.

BUG=29004
TEST=WriteError

Review URL: http://codereview.chromium.org/450025

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@33404 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 45 insertions, 25 deletions

diff --git a/net/flip/flip_session.cc b/net/flip/flip_session.cc
index 31fe46a..fc446f4 100644
--- a/net/flip/flip_session.cc
+++ b/net/flip/flip_session.cc
@@ -169,13 +169,13 @@ FlipSession::FlipSession(const std::string& host, HttpNetworkSession* session)
           write_callback_(this, &FlipSession::OnWriteComplete)),
       domain_(host),
       session_(session),
-      connection_started_(false),
-      connection_ready_(false),
       read_buffer_(new IOBuffer(kReadBufferSize)),
       read_pending_(false),
       stream_hi_water_mark_(1),  // Always start at 1 for the first stream id.
       write_pending_(false),
-      delayed_write_pending_(false) {
+      delayed_write_pending_(false),
+      error_(OK),
+      state_(IDLE) {
   // TODO(mbelshe): consider randomization of the stream_hi_water_mark.
 
   flip_framer_.set_visitor(this);
@@ -214,10 +214,10 @@ net::Error FlipSession::Connect(const std::string& group_name,
   DCHECK(priority >= FLIP_PRIORITY_HIGHEST && priority < FLIP_PRIORITY_LOWEST);
 
   // If the connect process is started, let the caller continue.
-  if (connection_started_)
+  if (state_ > IDLE)
     return net::OK;
 
-  connection_started_ = true;
+  state_ = CONNECTING;
 
   static StatsCounter flip_sessions("flip.sessions");
   flip_sessions.Increment();
@@ -393,9 +393,7 @@ void FlipSession::OnTCPConnect(int result) {
   LOG(INFO) << "Flip socket connected (result=" << result << ")";
 
   if (result != net::OK) {
-    net::Error err = static_cast<net::Error>(result);
-    CloseAllStreams(err);
-    session_->flip_session_pool()->Remove(this);
+    CloseSession(static_cast<net::Error>(result));
     return;
   }
 
@@ -420,7 +418,8 @@ void FlipSession::OnTCPConnect(int result) {
     int status = connection_.socket()->Connect(&ssl_connect_callback_, NULL);
     CHECK(status == net::ERR_IO_PENDING);
   } else {
-    connection_ready_ = true;
+    DCHECK_EQ(state_, CONNECTING);
+    state_ = CONNECTED;
 
     // Make sure we get any pending data sent.
     WriteSocketLater();
@@ -437,7 +436,8 @@ void FlipSession::OnSSLConnect(int result) {
     result = OK;   // TODO(mbelshe): pretend we're happy anyway.
 
   if (result == OK) {
-    connection_ready_ = true;
+    DCHECK_EQ(state_, CONNECTING);
+    state_ = CONNECTED;
 
     // After we've connected, send any data to the server, and then issue
     // our read.
@@ -460,9 +460,7 @@ void FlipSession::OnReadComplete(int bytes_read) {
 
   if (bytes_read <= 0) {
     // Session is tearing down.
-    net::Error err = static_cast<net::Error>(bytes_read);
-    CloseAllStreams(err);
-    session_->flip_session_pool()->Remove(this);
+    CloseSession(static_cast<net::Error>(bytes_read));
     return;
   }
 
@@ -524,13 +522,7 @@ void FlipSession::OnWriteComplete(int result) {
     in_flight_write_.release();
 
     // The stream is now errored.  Close it down.
-    CloseAllStreams(static_cast<net::Error>(result));
-    // TODO(mbelshe): we need to cleanup the session here as well.
-    //   Right now, but a read and a write can fail, and each will
-    //   remove the session from the pool, which trips asserts. We
-    //   need to cleanup the close logic so that we only call it
-    //   once.
-    //session_->flip_session_pool()->Remove(this);
+    CloseSession(static_cast<net::Error>(result));
   }
 }
 
@@ -575,7 +567,7 @@ void FlipSession::WriteSocket() {
 
   // If the socket isn't connected yet, just wait; we'll get called
   // again when the socket connection completes.
-  if (!connection_ready_)
+  if (state_ < CONNECTED)
     return;
 
   if (write_pending_)   // Another write is in progress still.
@@ -674,6 +666,20 @@ int FlipSession::GetNewStreamId() {
   return id;
 }
 
+void FlipSession::CloseSession(net::Error err) {
+  LOG(INFO) << "Flip::CloseSession(" << err << ")";
+
+  // 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;
+    error_ = err;
+    CloseAllStreams(err);
+    session_->flip_session_pool()->Remove(this);
+  }
+}
+
 void FlipSession::ActivateStream(FlipStream* stream) {
   const flip::FlipStreamId id = stream->stream_id();
   DCHECK(!IsStreamActive(id));
@@ -722,8 +728,7 @@ scoped_refptr<FlipStream> FlipSession::GetPushStream(const std::string& path) {
 
 void FlipSession::OnError(flip::FlipFramer* framer) {
   LOG(ERROR) << "FlipSession error: " << framer->error_code();
-  CloseAllStreams(net::ERR_UNEXPECTED);
-  Release();
+  CloseSession(net::ERR_UNEXPECTED);
 }
 
 void FlipSession::OnStreamFrameData(flip::FlipStreamId stream_id,
diff --git a/net/flip/flip_session.h b/net/flip/flip_session.h
index 7555b85..545b902 100644
--- a/net/flip/flip_session.h
+++ b/net/flip/flip_session.h
@@ -75,6 +75,13 @@ class FlipSession : public base::RefCounted<FlipSession>,
  protected:
   friend class FlipSessionPool;
 
+  enum State {
+    IDLE,
+    CONNECTING,
+    CONNECTED,
+    CLOSED
+  };
+
   // Provide access to the framer for testing.
   flip::FlipFramer* GetFramer() { return &flip_framer_; }
 
@@ -125,6 +132,10 @@ class FlipSession : public base::RefCounted<FlipSession>,
   // Get a new stream id.
   int GetNewStreamId();
 
+  // Closes this session.  This will close all active streams and mark
+  // the session as permanently closed.
+  void CloseSession(net::Error err);
+
   // Track active streams in the active stream list.
   void ActivateStream(FlipStream* stream);
   void DeactivateStream(flip::FlipStreamId id);
@@ -149,8 +160,6 @@ class FlipSession : public base::RefCounted<FlipSession>,
 
   // The socket handle for this session.
   ClientSocketHandle connection_;
-  bool connection_started_;  // Is the connect process started.
-  bool connection_ready_;  // Is the connection ready for use.
 
   // The read buffer used to read data from the socket.
   scoped_refptr<IOBuffer> read_buffer_;
@@ -192,6 +201,12 @@ class FlipSession : public base::RefCounted<FlipSession>,
   // Flip Frame state.
   flip::FlipFramer flip_framer_;
 
+  // If an error has occurred on the session, the session is effectively
+  // dead.  Record this error here.  When no error has occurred, |error_| will
+  // be OK.
+  net::Error error_;
+  State state_;
+
   static bool use_ssl_;
 };