Add some implementation documentation

This might make it a little bit easier for a new reader/maintainer to understand what this code is doing. Also, link to a couple application-level bug reports about probable misbehaviors related to the deadlock-detection code.
python · brettcannon · Jan 21, 2023 · Jun 27, 2022 · Jun 27, 2022 · Jun 30, 2022
commit 2a4d4f3d4b48bbfa12f1a461d1a8422ef4d99d1d
@@ -71,9 +71,30 @@ class _ModuleLock:
     def __init__(self, name):
         self.lock = _thread.allocate_lock()
         self.wakeup = _thread.allocate_lock()
+        # The name of the module for which this is a lock.
         self.name = name
+
+        # Either None if this lock is not owned by any thread or the thread
+        # identifier for the owning thread.
         self.owner = None
+
+        # This is a count of the number of times the owning thread has
+        # acquired this lock.  This supports RLock-like ("re-entrant lock")
+        # behavior, necessary in case a single thread is following a circular
+        # import dependency and needs to take the lock for a single module
+        # more than once.
         self.count = 0
+
+        # This is a count of the number of threads that are blocking on
+        # `self.wakeup.acquire()` to try to get their turn holding this module
+        # lock.  When the module lock is released, if this is greater than
+        # zero, it is decremented and `self.wakeup` is released one time.  The
+        # intent is that this will let one other thread make more progress on
+        # acquiring this module lock.  This repeats until all the threads have
+        # gotten a turn.
+        #
+        # This is incremented in `self.acquire` when a thread notices it is
+        # going to have to wait for another thread to finish.
         self.waiters = 0
 
     def has_deadlock(self):
@@ -107,17 +128,64 @@ def acquire(self):
         _blocking_on[tid] = self
         try:
             while True:
+                # Protect interaction with state on self with a per-module
+                # lock.  This makes it safe for more than one thread to try to
+                # acquire the lock for a single module at the same time.
                 with self.lock:
                     if self.count == 0 or self.owner == tid:
+                        # If the lock for this module is unowned then we can
+                        # take the lock immediately and succeed.  If the lock
+                        # for this module is owned by the running thread then
+                        # we can also allow the acquire to succeed.  This
+                        # supports circular imports (thread T imports module A
+                        # which imports module B which imports module A).
                         self.owner = tid
                         self.count += 1
                         return True
+
+
+                    # At this point we know the lock is held (because count !=
+                    # 0) by another thread (because owner != tid).  We'll have
+                    # to get in line to take the module lock.
+
+                    # But first, check to see if this thread would create a
+                    # deadlock by acquiring this module lock.  If it would
+                    # then just stop with an error.
+                    #
+                    # XXX It's not clear who is expected to handle this error.
+                    # There is one handler in _lock_unlock_module but many
+                    # times this method is called when entering the context
+                    # manager _ModuleLockManager instead - so _DeadlockError
+                    # will just propagate up to application code.
+                    #
+                    # This seems to be more than just a hypothetical -
+                    # https://stackoverflow.com/questions/59509154
+                    # https://github.com/encode/django-rest-framework/issues/7078
                     if self.has_deadlock():
                         raise _DeadlockError('deadlock detected by %r' % self)
+
+
+                    # Check to see if we're going to be able to acquire the
+                    # lock.  If we are going to have to wait then increment
+                    # the waiters so `self.release` will know to unblock us
+                    # later on.  We do this part non-blockingly so we don't
+                    # get stuck here before we increment waiters.  We have
+                    # this extra acquire call (in addition to the one below,
+                    # outside the self.lock context manager) to make sure
+                    # self.wakeup is held when the next acquire is called (so
+                    # we block).  This is probably needlessly complex and we
+                    # should just take self.wakeup in the return codepath
+                    # above.
                     if self.wakeup.acquire(False):
                         self.waiters += 1
-                # Wait for a release() call
+
+                # Now blockingly take the lock.  This won't complete until the
+                # thread holding this lock (self.owner) calls self.release.
                 self.wakeup.acquire()
+
+                # Taking it has served its purpose (making us wait) so we can
+                # give it up now.  We'll take it non-blockingly again on the
+                # next iteration around this while loop.
                 self.wakeup.release()
         finally:
             del _blocking_on[tid]