[tor-commits] [stem/master] Concurrency is still hard

[atagar at torproject.org]

commit 4f8be72088e40bb4a6b62e9e39a4c3d9121ec472
Author: Damian Johnson <atagar at torproject.org>
Date:   Sun Apr 22 20:40:53 2012 -0700

    Concurrency is still hard
    
    It felt like the handle_close flag was hacky and now I know why. If recv()
    tried to call close() while a send() call was in flight and would also
    eventually call close() then we'd encounter a similer type of deadlock as
    commit d5162f4 tried to fix.
    
    This did not come up for control port connections, but it did frequently occure
    for control sockets (as seen by deadlock when running the RUN_SOCKET target).
    This did not always occure because the recv() sometimes acquired the send_lock
    before the next send() call, making everything proceed correctly.
    
    Fixing this by having recv() make a non-blocking attempt to get the send_lock.
    If it works then we know that we can safely close, and if we can't acquire the
    lock then we know that a send() or close() call is already in progress and can
    leave the closing to them.
    
    This does introduce a possible issue where the send() call succeeds, the recv()
    call fails with a SocketClosed, and we're left in a state of being alive.
    However, this is both a weird use case (how can the send() work if the socket
    is closed?) and also not strictly wrong (raising a SocketClosed does not mean
    that we've finished shutting ourselves down). It's a little tempting to add a
    dedicated close_lock to account for this, but after looking into that I
    realized that this would make the concurrency far more convoluted.
---
 stem/socket.py |   27 +++++++++++++++++----------
 1 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/stem/socket.py b/stem/socket.py
index 55129f3..6f0bfda 100644
--- a/stem/socket.py
+++ b/stem/socket.py
@@ -89,9 +89,6 @@ class ControlSocket:
     self._socket, self._socket_file = None, None
     self._is_alive = False
     
-    # indicates that we're in the midst of calling close()
-    self._handling_close = False
-    
     # Tracks sending and receiving separately. This should be safe, and doing
     # so prevents deadlock where we block writes because we're waiting to read
     # a message that isn't coming.
@@ -149,10 +146,24 @@ class ControlSocket:
         return recv_message(socket_file)
       except SocketClosed, exc:
         # If recv_message raises a SocketClosed then we should properly shut
-        # everything down. However, if this was caused *from* a close call
-        # and it's joining on our thread then this would risk deadlock.
+        # everything down. However, there's a couple cases where this will
+        # cause deadlock...
+        #
+        # * this socketClosed was *caused by* a close() call, which is joining
+        #   on our thread
+        #
+        # * a send() call that's currently in flight is about to call close(),
+        #   also attempting to join on us
+        #
+        # To resolve this we make a non-blocking call to acquire the send lock.
+        # If we get it then great, we can close safely. If not then one of the
+        # above are in progress and we leave the close to them.
+        
+        if self.is_alive():
+          if self._send_lock.acquire(False):
+            self.close()
+            self._send_lock.release()
         
-        if self.is_alive() and not self._handling_close: self.close()
         raise exc
   
   def is_alive(self):
@@ -202,8 +213,6 @@ class ControlSocket:
     Shuts down the socket. If it's already closed then this is a no-op.
     """
     
-    self._handling_close = True
-    
     with self._send_lock:
       # Function is idempotent with one exception: we notify _close() if this
       # is causing our is_alive() state to change.
@@ -235,8 +244,6 @@ class ControlSocket:
       
       if is_change:
         self._close()
-      
-      self._handling_close = False
   
   def _get_send_lock(self):
     """