Update comments and add assertions.

Also describe _PyRWMutex using bit patterns instead of ASCII art box. This matches how PyMutex is described.
python · ericsnowcurrently · Dec 16, 2023 · Dec 7, 2023 · Dec 7, 2023 · Dec 14, 2023
commit e8419d783ca38f7542c8c627b88ea841e557291f
diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h
@@ -214,23 +214,26 @@ _PyOnceFlag_CallOnce(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg)
 }
 
 // A readers-writer (RW) lock. The lock supports multiple concurrent readers or
-// a single writer. The lock is write-preferring: if a writer is waiting, then
-// new readers will be blocked. This avoids starvation of writers.
+// a single writer. The lock is write-preferring: if a writer is waiting while
+// the lock is read-locked then, new readers will be blocked. This avoids
+// starvation of writers.
 //
-// The bitfield is structured as follows:
+// In C++, the equivalent synchronization primitive is std::shared_mutex
+// with shared ("read") and exclusive ("write") locking.
 //
-// N ...                                     2  1  0
-// +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
-// |                  reader_count           |P |WL|
-// +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
+// The two least significant bits are used to indicate if the lock is
+// write-locked and if there are parked threads (either readers or writers)
+// waiting to acquire the lock. The remaining bits are used to indicate the
+// number of readers holding the lock.
 //
-// The least significant bit (WL / _Py_WRITE_LOCKED) indicates if the mutex is
-// write-locked. The next bit (PK/ _Py_HAS_PARKED) indicates if there are
-// parked threads (either readers or writers). The remaining bits
-// (reader_count) indicate the number of readers holding the lock.
+// 0b000..00000: unlocked
+// 0bnnn..nnn00: nnn..nnn readers holding the lock
+// 0bnnn..nnn10: nnn..nnn readers holding the lock and a writer is waiting
+// 0b00000..001: write-locked
+// 0b00000..011: write-locked and readers or other writers are waiting
 //
-// Note that reader_count must be zero if WL is set, and vice versa. The lock
-// can only be held by readers or a writer, but not both.
+// Note that reader_count must be zero if the lock is held by a writer, and
+// vice versa. The lock can only be held by readers or a writer, but not both.
 //
 // The design is optimized for simplicity of the implementation. The lock is
 // not fair: if fairness is desired, use an additional PyMutex to serialize
@@ -239,11 +242,11 @@ typedef struct {
     uintptr_t bits;
 } _PyRWMutex;
 
-// Read lock
+// Read lock (i.e., shared lock)
 PyAPI_FUNC(void) _PyRWMutex_RLock(_PyRWMutex *rwmutex);
 PyAPI_FUNC(void) _PyRWMutex_RUnlock(_PyRWMutex *rwmutex);
 
-// Write lock
+// Write lock (i.e., exclusive lock)
 PyAPI_FUNC(void) _PyRWMutex_Lock(_PyRWMutex *rwmutex);
 PyAPI_FUNC(void) _PyRWMutex_Unlock(_PyRWMutex *rwmutex);
 

diff --git a/Python/lock.c b/Python/lock.c
@@ -388,14 +388,16 @@ _PyRWMutex_RLock(_PyRWMutex *rwmutex)
     uintptr_t bits = _Py_atomic_load_uintptr_relaxed(&rwmutex->bits);
     for (;;) {
         if ((bits & _Py_WRITE_LOCKED)) {
-            // The lock is write-locked.
+            // A writer already holds the lock.
             bits = rwmutex_set_parked_and_wait(rwmutex, bits);
             continue;
         }
         else if ((bits & _Py_HAS_PARKED)) {
-            // There is at least one waiting writer. We can't grab the lock
-            // because we don't want to starve the writer. Instead, we park
-            // ourselves and wait for the writer to eventually wake us up.
+            // Reader(s) hold the lock, but there is at least one waiting
+            // writer. We can't grab the lock because we don't want to starve
+            // the writer. Instead, we park ourselves and wait for the writer
+            // to eventually wake us up.
+            assert(rwmutex_reader_count(bits) > 0);
             bits = rwmutex_set_parked_and_wait(rwmutex, bits);
             continue;
         }
@@ -416,6 +418,7 @@ void
 _PyRWMutex_RUnlock(_PyRWMutex *rwmutex)
 {
     uintptr_t bits = _Py_atomic_add_uintptr(&rwmutex->bits, -(1 << _PyRWMutex_READER_SHIFT));
+    assert(rw_mutex_reader_count(bits) > 0 && "lock was not read-locked");
     bits -= (1 << _PyRWMutex_READER_SHIFT);
 
     if (rwmutex_reader_count(bits) == 0 && (bits & _Py_HAS_PARKED)) {