svn commit: r1187108 - in /trafficserver/traffic/trunk: ./ iocore/net/ proxy/http/

[am...@apache.org]

Author: amc
Date: Thu Oct 20 22:39:49 2011
New Revision: 1187108

URL: http://svn.apache.org/viewvc?rev=1187108&view=rev
Log:
TS-934 Wrapper for NetVConnection to allow safer locking of connection objects between threads.

Modified:
    trafficserver/traffic/trunk/CHANGES
    trafficserver/traffic/trunk/iocore/net/I_NetVConnection.h
    trafficserver/traffic/trunk/iocore/net/NetVConnection.cc
    trafficserver/traffic/trunk/iocore/net/UnixNet.cc
    trafficserver/traffic/trunk/iocore/net/UnixNetVConnection.cc
    trafficserver/traffic/trunk/proxy/http/HttpSM.cc
    trafficserver/traffic/trunk/proxy/http/HttpServerSession.cc
    trafficserver/traffic/trunk/proxy/http/HttpServerSession.h

Modified: trafficserver/traffic/trunk/CHANGES
URL: http://svn.apache.org/viewvc/trafficserver/traffic/trunk/CHANGES?rev=1187108&r1=1187107&r2=1187108&view=diff
==============================================================================
--- trafficserver/traffic/trunk/CHANGES (original)
+++ trafficserver/traffic/trunk/CHANGES Thu Oct 20 22:39:49 2011
@@ -1,5 +1,10 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache Traffic Server 3.1.1
+
+  *) [TS-934] Added some wrapping around NetVConnection for server
+   handling so that connection objects can be safely locked across
+   threads.
+
   *) [TS-991] Fixed race / stall condition for WCCP during restart.
 
   *) [TS-985] ts/ts.h uses C++ comments, which are technically not C.

Modified: trafficserver/traffic/trunk/iocore/net/I_NetVConnection.h
URL: http://svn.apache.org/viewvc/trafficserver/traffic/trunk/iocore/net/I_NetVConnection.h?rev=1187108&r1=1187107&r2=1187108&view=diff
==============================================================================
--- trafficserver/traffic/trunk/iocore/net/I_NetVConnection.h (original)
+++ trafficserver/traffic/trunk/iocore/net/I_NetVConnection.h Thu Oct 20 22:39:49 2011
@@ -402,6 +402,7 @@ public:
 
   unsigned int attributes;
   EThread *thread;
+  volatile uint32_t generation; ///< Counter for re-use checking.
 
   /// PRIVATE: The public interface is VIO::reenable()
   virtual void reenable(VIO * vio) = 0;
@@ -459,6 +460,88 @@ public:
     is_other_side_transparent = value;
   }
 
+  /** Struct for holding a reference to a connection.
+      The problem is that connections are accessed across thread boundaries
+      and re-used so that a pointer to a VC can become not only stale
+      but a reference to another connection entirely aynchronously. The
+      lock is contained within the connection so it can't be reliably accessed
+      from just a raw pointer.
+  */
+  struct Handle {
+    typedef Handle self; ///< Self reference type.
+
+    NetVConnection* _vc; ///< Pointer to the VC.
+    uint32_t _generation; ///< Generation of VC when assigned.
+    ProxyMutexPtr _mutex; ///< Shared mutex with VC at time of assignment.
+    
+    /// Default constructor.
+    Handle() : _vc(0), _generation(0) {}
+    /// Construct from VC.
+    Handle(NetVConnection* vc) { *this = vc; }
+    /// Assign VC.
+    self& operator = (NetVConnection* vc) {
+      _vc = vc;
+      if (vc) {
+        _mutex = vc->mutex;
+        _generation = vc->generation;
+      } else {
+        _generation = 0;
+        _mutex.clear();
+      }
+      return *this;
+    }
+
+    /// Dereference operator.
+    NetVConnection* operator -> () { return _vc; }
+    /// Get referenced VC.
+    NetVConnection* getVC() { return _vc; }
+    /// Get pointer mutex.
+    ProxyMutexPtr& getMutex() { return _mutex; }
+    /** Check if this reference is still valid.
+        This should only be called under lock of @a _mutex.
+        @return @c true 
+    */
+    bool isValid() {
+      return _vc && // have a connection
+        _mutex.m_ptr == _vc->mutex.m_ptr && // and it's still the same mutex
+        _generation == _vc->generation // and same generation.
+        ;
+    }
+    /** Invoke @c do_io_close on the underlying VC.
+        This handles locking and validating the VC. If a lock can be obtained
+        then the operation is performed immediately. If not then it is
+        scheduled on the thread holding the lock.
+    */
+    void do_locked_io_close(int lerrno = -1);
+  };
+
+  /** A subclass of @c Handle that can be used in @c IntrusivePtrList.
+   */
+
+  /** Use this to schedule closing the connection.
+
+      This can be needed if the connection is part of a transaction
+      but being handled in another thread. The continuation mutex is a
+      shared reference to the VC mutex so that it is not garbage
+      collected and can be checked against the VC after being locked.
+  */
+  struct Close_callback : public Continuation {
+    typedef Continuation super; ///< Super type.
+    typedef Close_callback self; ///< Self reference type.
+
+    Handle _vcptr; ///< Reference to the target VC.
+    int _lerrno; ///< Errno value to pass to @c do_io_close.
+
+    int try_close(int, Event *);
+
+    Close_callback(Handle const& vcptr, int lerrno = -1)
+      : super(&*(vcptr._mutex))
+      , _vcptr(vcptr)
+      , _lerrno(lerrno) {
+      SET_HANDLER(&self::try_close);
+    }
+  };
+
 private:
   NetVConnection(const NetVConnection &);
   NetVConnection & operator =(const NetVConnection &);
@@ -485,14 +568,15 @@ NetVConnection::NetVConnection():
   VConnection(NULL),
   attributes(0),
   thread(NULL),
+  generation(1),
   got_local_addr(0),
   got_remote_addr(0),
   is_internal_request(false),
   is_transparent(false),
   is_other_side_transparent(false)
 {
-  memset(&local_addr, 0, sizeof(local_addr));
-  memset(&remote_addr, 0, sizeof(remote_addr));
+  ink_zero(local_addr);
+  ink_zero(remote_addr);
 }
 
 #endif

Modified: trafficserver/traffic/trunk/iocore/net/NetVConnection.cc
URL: http://svn.apache.org/viewvc/trafficserver/traffic/trunk/iocore/net/NetVConnection.cc?rev=1187108&r1=1187107&r2=1187108&view=diff
==============================================================================
--- trafficserver/traffic/trunk/iocore/net/NetVConnection.cc (original)
+++ trafficserver/traffic/trunk/iocore/net/NetVConnection.cc Thu Oct 20 22:39:49 2011
@@ -44,3 +44,64 @@ NetVConnection::cancel_OOB()
   return;
 }
 
+int
+NetVConnection::Close_callback::try_close(int, Event *) {
+  // Mutex for this continuation is the original VC mutex so if we're
+  // being dispatched, it should be locked enough to check.
+  if (_vcptr.isValid()) {
+    _vcptr->do_io_close(_lerrno);
+  }
+  delete this;
+  return EVENT_DONE;
+}
+
+void
+NetVConnection::Handle::do_locked_io_close(int lerrno) {
+  EThread* t = this_ethread();
+  // If the VC or the mutex isn't there, we're done.
+  bool handled = ! (_vc && _mutex.m_ptr);
+  /* This loop is an attempt to avoid race conditions.  We need to
+     shut down the VC without colliding with another thread.
+
+     First we try to get the lock. If that works then if our
+     reference is still valid we can do the shutdown directly. If
+     it's not valid we assume that someone else already did the
+     shutdown (such a collision being precisely what we're trying to
+     avoid).
+
+     If we can't get the lock, we need to schedule a retry on the
+     thread that owns the lock. However, we're racing at this because
+     without the lock the VC or the lock itself can change. In
+     particular, the thread owning the lock can drop it and leave us
+     without a valid thread. So we grab what we can and check it. If
+     we have a thread, we schedule there. Otherwise the lock has been
+     dropped so we cycle back to trying the lock.
+
+     We presume that there's extremely little chance of this just
+     missing the lock happening and an infinitesmal chance of it
+     happening more than once. So the loop shouldn't go more than once
+     or twice and then only vary rarely. Further, based on experience
+     in a high throughput environment we should expect to not get the
+     lock in the first place only in rare circumstances. For most
+     situations and in most environments we should not ever do the
+     scheduling. Unfortunately it does happen under some circumstances
+     so we have to deal with it.
+ */
+  while (!handled) {
+    MUTEX_TRY_LOCK(lock, _mutex, t);
+    if (lock) {
+      if (this->isValid()) _vc->do_io_close(lerrno);
+      handled = true;
+    } else {
+      // Gosh, it's probable that the VC has gone bad at this point but
+      // we can't reliably detect that without the lock.
+      EThread* ot = _mutex->thread_holding;
+      ink_assert(ot != t); // we should have the lock in that case.
+      if (ot) {
+        // schedule shutdown on other thread.
+        ot->schedule_imm(NEW(new UnixNetVConnection::Close_callback(*this, lerrno)));
+        handled = true;
+      }
+    }
+  }
+}

Modified: trafficserver/traffic/trunk/iocore/net/UnixNet.cc
URL: http://svn.apache.org/viewvc/trafficserver/traffic/trunk/iocore/net/UnixNet.cc?rev=1187108&r1=1187107&r2=1187108&view=diff
==============================================================================
--- trafficserver/traffic/trunk/iocore/net/UnixNet.cc (original)
+++ trafficserver/traffic/trunk/iocore/net/UnixNet.cc Thu Oct 20 22:39:49 2011
@@ -51,6 +51,7 @@ struct InactivityCop : public Continuati
     forl_LL(UnixNetVConnection, vc, nh->open_list)
       nh->cop_list.push(vc);
     while (UnixNetVConnection *vc = nh->cop_list.pop()) {
+      MUTEX_LOCK(lock, vc->mutex, this_ethread());
       if (vc->closed) {
         close_UnixNetVConnection(vc, e->ethread);
         continue;

Modified: trafficserver/traffic/trunk/iocore/net/UnixNetVConnection.cc
URL: http://svn.apache.org/viewvc/trafficserver/traffic/trunk/iocore/net/UnixNetVConnection.cc?rev=1187108&r1=1187107&r2=1187108&view=diff
==============================================================================
--- trafficserver/traffic/trunk/iocore/net/UnixNetVConnection.cc (original)
+++ trafficserver/traffic/trunk/iocore/net/UnixNetVConnection.cc Thu Oct 20 22:39:49 2011
@@ -1106,11 +1106,12 @@ UnixNetVConnection::free(EThread *t)
 {
   NET_SUM_GLOBAL_DYN_STAT(net_connections_currently_open_stat, -1);
   // clear variables for reuse
+  ++generation;
+  this->mutex.clear();
   got_remote_addr = 0;
   got_local_addr = 0;
   read.vio.mutex.clear();
   write.vio.mutex.clear();
-  this->mutex.clear();
   flags = 0;
   accept_port = 0;
   SET_CONTINUATION_HANDLER(this, (NetVConnHandler) & UnixNetVConnection::startEvent);

Modified: trafficserver/traffic/trunk/proxy/http/HttpSM.cc
URL: http://svn.apache.org/viewvc/trafficserver/traffic/trunk/proxy/http/HttpSM.cc?rev=1187108&r1=1187107&r2=1187108&view=diff
==============================================================================
--- trafficserver/traffic/trunk/proxy/http/HttpSM.cc (original)
+++ trafficserver/traffic/trunk/proxy/http/HttpSM.cc Thu Oct 20 22:39:49 2011
@@ -5931,8 +5931,8 @@ HttpSM::kill_this()
     if (second_cache_sm)
       second_cache_sm->end_both();
     transform_cache_sm.end_both();
-    tunnel.deallocate_buffers();
     vc_table.cleanup_all();
+    tunnel.deallocate_buffers();
 
     // It possible that a plugin added transform hook
     //   but the hook never executed due to a client abort

Modified: trafficserver/traffic/trunk/proxy/http/HttpServerSession.cc
URL: http://svn.apache.org/viewvc/trafficserver/traffic/trunk/proxy/http/HttpServerSession.cc?rev=1187108&r1=1187107&r2=1187108&view=diff
==============================================================================
--- trafficserver/traffic/trunk/proxy/http/HttpServerSession.cc (original)
+++ trafficserver/traffic/trunk/proxy/http/HttpServerSession.cc Thu Oct 20 22:39:49 2011
@@ -41,7 +41,7 @@ ClassAllocator<HttpServerSession> httpSe
 void
 HttpServerSession::destroy()
 {
-  ink_release_assert(server_vc == NULL);
+  ink_release_assert(server_vc.getVC() == NULL);
   ink_assert(read_buffer);
   ink_assert(server_trans_stat == 0);
   magic = HTTP_SS_MAGIC_DEAD;
@@ -119,7 +119,7 @@ HttpServerSession::do_io_close(int alerr
     this->server_trans_stat--;
   }
 
-  server_vc->do_io_close(alerrno);
+  server_vc.do_locked_io_close(alerrno);
   Debug("http_ss", "[%" PRId64 "] session closed", con_id);
   server_vc = NULL;
 

Modified: trafficserver/traffic/trunk/proxy/http/HttpServerSession.h
URL: http://svn.apache.org/viewvc/trafficserver/traffic/trunk/proxy/http/HttpServerSession.h?rev=1187108&r1=1187107&r2=1187108&view=diff
==============================================================================
--- trafficserver/traffic/trunk/proxy/http/HttpServerSession.h (original)
+++ trafficserver/traffic/trunk/proxy/http/HttpServerSession.h Thu Oct 20 22:39:49 2011
@@ -110,7 +110,7 @@ public:
   void attach_hostname(const char *hostname);
   NetVConnection *get_netvc()
   {
-    return server_vc;
+    return server_vc.getVC();
   };
 
   // Keys for matching hostnames
@@ -160,7 +160,7 @@ public:
 private:
   HttpServerSession(HttpServerSession &);
 
-  NetVConnection *server_vc;
+  NetVConnection::Handle server_vc;
   int magic;
 
   IOBufferReader *buf_reader;