Fix issue with readerwritelock usage in basilisp (#722) (#731)

Hi,

could you please review patch to fix what it seems to be a potential
issue with how readwrite locks are used in basilisp. It addresses #722.

This was discovered while testing of the upcoming nrepl-server described
in #723. The nrepl-server tests which are using threads were failing due
to trying to release an rlock which has been already released, and is
fixed as described in #722 by using `with self._lock.gen_rlock()`
instead of a single instance of `with self._rlock()`. The error was
```
    File "/home/runner/work/basilisp/basilisp/.tox/py39/lib/python3.9/site-packages/basilisp/lang/runtime.py", line 706, in find
    return v
   File "/home/runner/work/basilisp/basilisp/.tox/py39/lib/python3.9/site-packages/readerwriterlock/rwlock.py", line 49, in __exit__
    self.release()
   File "/home/runner/work/basilisp/basilisp/.tox/py39/lib/python3.9/site-packages/readerwriterlock/rwlock.py", line 344, in release
    if not self.v_locked: raise RELEASE_ERR_CLS(RELEASE_ERR_MSG)
 RuntimeError: release unlocked lock
```

Thanks

Co-authored-by: ikappaki <[email protected]>

Author: ikappaki

CHANGELOG.md

@@ -17,10 +17,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  * Fix `(is (= exp act))` should only evaluate its args once on failure (#712).
  * Fix issue with `with` failing with a traceback error when an exception is thrown (#714).
  * Fix issue with `sort-*` family of funtions returning an error on an empty seq (#716).
- * Fix issue with `intern` failing when used (#725).
+ * Fix issue with `intern` failing when used (#725). 
  * Fix issue with `ns` not being available after `in-ns` on the REPL (#718).
  * Fixed issue with import modules aliasing using ns eval (#719).
  * Fix issue with `ns-resolve` throwing an error on macros (#720).
+ * Fix issue with py module `readerwritelock` locks handling (#722).
 
 ## [v0.1.0a2]
 ### Added

src/basilisp/lang/atom.py

@@ -10,7 +10,7 @@
 
 
 class Atom(RefBase[T], Generic[T]):
-    __slots__ = ("_meta", "_state", "_rlock", "_wlock", "_watches", "_validator")
+    __slots__ = ("_meta", "_state", "_lock", "_watches", "_validator")
 
     def __init__(
         self,
@@ -20,17 +20,15 @@ def __init__(
     ) -> None:
         self._meta: Optional[IPersistentMap] = meta
         self._state = state
-        lock = RWLockFair()
-        self._rlock = lock.gen_rlock()
-        self._wlock = lock.gen_wlock()
+        self._lock = RWLockFair()
         self._watches = PersistentMap.empty()
         self._validator = validator
 
         if validator is not None:
             self._validate(state)
 
     def _compare_and_set(self, old: T, new: T) -> bool:
-        with self._wlock:
+        with self._lock.gen_wlock():
             if self._state != old:
                 return False
             self._state = new
@@ -48,7 +46,7 @@ def compare_and_set(self, old: T, new: T) -> bool:
 
     def deref(self) -> T:
         """Return the state stored within the Atom."""
-        with self._rlock:
+        with self._lock.gen_rlock():
             return self._state
 
     def reset(self, v: T) -> T:

src/basilisp/lang/reference.py

@@ -1,6 +1,6 @@
 from typing import Any, Callable, Optional, TypeVar
 
-from readerwriterlock.rwlock import Lockable
+from readerwriterlock.rwlock import RWLockable
 
 from basilisp.lang import keyword as kw
 from basilisp.lang import map as lmap
@@ -33,24 +33,23 @@ class ReferenceBase(IReference):
     `basilisp.lang.runtime.Namespace` objects are the only objects which are
     `IReference` objects without also being `IRef` objects.
 
-    Implementers must have the `_rlock`, `_wlock`, and `_meta` properties defined."""
+    Implementers must have the `_lock` and `_meta` properties defined."""
 
-    _rlock: Lockable
-    _wlock: Lockable
+    _lock: RWLockable
     _meta: Optional[IPersistentMap]
 
     @property
     def meta(self) -> Optional[IPersistentMap]:
-        with self._rlock:
+        with self._lock.gen_rlock():
             return self._meta
 
     def alter_meta(self, f: AlterMeta, *args) -> Optional[IPersistentMap]:
-        with self._wlock:
+        with self._lock.gen_wlock():
             self._meta = f(self._meta, *args)
             return self._meta
 
     def reset_meta(self, meta: Optional[IPersistentMap]) -> Optional[IPersistentMap]:
-        with self._wlock:
+        with self._lock.gen_wlock():
             self._meta = meta
             return meta
 
@@ -72,7 +71,7 @@ class RefBase(IRef[T], ReferenceBase):
     _watches: IPersistentMap
 
     def add_watch(self, k: RefWatchKey, wf: RefWatcher) -> "RefBase[T]":
-        with self._wlock:
+        with self._lock.gen_wlock():
             self._watches = self._watches.assoc(k, wf)
             return self
 
@@ -81,7 +80,7 @@ def _notify_watches(self, old: Any, new: Any):
             wf(k, self, old, new)
 
     def remove_watch(self, k: RefWatchKey) -> "RefBase[T]":
-        with self._wlock:
+        with self._lock.gen_wlock():
             self._watches = self._watches.dissoc(k)
             return self
 

src/basilisp/lang/runtime.py

@@ -228,8 +228,7 @@ class Var(RefBase):
         "_is_bound",
         "_tl",
         "_meta",
-        "_rlock",
-        "_wlock",
+        "_lock",
         "_watches",
         "_validator",
     )
@@ -248,9 +247,7 @@ def __init__(
         self._is_bound = False
         self._tl = None
         self._meta = meta
-        lock = RWLockFair()
-        self._rlock = lock.gen_rlock()
-        self._wlock = lock.gen_wlock()
+        self._lock = RWLockFair()
         self._watches = lmap.PersistentMap.empty()
         self._validator = None
 
@@ -281,7 +278,7 @@ def dynamic(self) -> bool:
         return self._dynamic
 
     def set_dynamic(self, dynamic: bool) -> None:
-        with self._wlock:
+        with self._lock.gen_wlock():
             if dynamic == self._dynamic:
                 return
 
@@ -309,18 +306,18 @@ def _set_root(self, newval) -> None:
 
     @property
     def root(self):
-        with self._rlock:
+        with self._lock.gen_rlock():
             return self._root
 
     def bind_root(self, val) -> None:
         """Atomically update the root binding of this Var to val."""
-        with self._wlock:
+        with self._lock.gen_wlock():
             self._set_root(val)
 
     def alter_root(self, f, *args) -> None:
         """Atomically alter the root binding of this Var to the result of calling
         f with the existing root value and any additional arguments."""
-        with self._wlock:
+        with self._lock.gen_wlock():
             self._set_root(f(self._root, *args))
 
     def push_bindings(self, val):
@@ -347,7 +344,7 @@ def value(self):
 
         For non-dynamic Vars, this will just be the root. For dynamic Vars, this will
         be any thread-local binding if one is defined. Otherwise, the root value."""
-        with self._rlock:
+        with self._lock.gen_rlock():
             if self._dynamic:
                 assert self._tl is not None
                 if len(self._tl.bindings) > 0:
@@ -359,7 +356,7 @@ def set_value(self, v) -> None:
 
         If the Var is not dynamic, this is equivalent to binding the root value. If the
         Var is dynamic, this will set the thread-local bindings for the Var."""
-        with self._wlock:
+        with self._lock.gen_wlock():
             if self._dynamic:
                 assert self._tl is not None
                 self._validate(v)
@@ -551,8 +548,7 @@ class Namespace(ReferenceBase):
         "_name",
         "_module",
         "_meta",
-        "_rlock",
-        "_wlock",
+        "_lock",
         "_interns",
         "_refers",
         "_aliases",
@@ -567,9 +563,7 @@ def __init__(
         self._module = Maybe(module).or_else(lambda: _new_module(name.as_python_sym()))
 
         self._meta: Optional[IPersistentMap] = None
-        lock = RWLockFair()
-        self._rlock = lock.gen_rlock()
-        self._wlock = lock.gen_wlock()
+        self._lock = RWLockFair()
 
         self._aliases: NamespaceMap = lmap.PersistentMap.empty()
         self._imports: ModuleMap = lmap.map(
@@ -605,36 +599,36 @@ def module(self, m: BasilispModule):
     def aliases(self) -> NamespaceMap:
         """A mapping between a symbolic alias and another Namespace. The
         fully qualified name of a namespace is also an alias for itself."""
-        with self._rlock:
+        with self._lock.gen_rlock():
             return self._aliases
 
     @property
     def imports(self) -> ModuleMap:
         """A mapping of names to Python modules imported into the current
         namespace."""
-        with self._rlock:
+        with self._lock.gen_rlock():
             return self._imports
 
     @property
     def import_aliases(self) -> AliasMap:
         """A mapping of a symbolic alias and a Python module name."""
-        with self._rlock:
+        with self._lock.gen_rlock():
             return self._import_aliases
 
     @property
     def interns(self) -> VarMap:
         """A mapping between a symbolic name and a Var. The Var may point to
         code, data, or nothing, if it is unbound. Vars in `interns` are
         interned in _this_ namespace."""
-        with self._rlock:
+        with self._lock.gen_rlock():
             return self._interns
 
     @property
     def refers(self) -> VarMap:
         """A mapping between a symbolic name and a Var. Vars in refers are
         interned in another namespace and are only referred to without an
         alias in this namespace."""
-        with self._rlock:
+        with self._lock.gen_rlock():
             return self._refers
 
     def __repr__(self):
@@ -665,41 +659,41 @@ def require(self, ns_name: str, *aliases: sym.Symbol) -> BasilispModule:
 
     def add_alias(self, namespace: "Namespace", *aliases: sym.Symbol) -> None:
         """Add Symbol aliases for the given Namespace."""
-        with self._wlock:
+        with self._lock.gen_wlock():
             new_m = self._aliases
             for alias in aliases:
                 new_m = new_m.assoc(alias, namespace)
             self._aliases = new_m
 
     def get_alias(self, alias: sym.Symbol) -> "Optional[Namespace]":
         """Get the Namespace aliased by Symbol or None if it does not exist."""
-        with self._rlock:
+        with self._lock.gen_rlock():
             return self._aliases.val_at(alias, None)
 
     def remove_alias(self, alias: sym.Symbol) -> None:
         """Remove the Namespace aliased by Symbol. Return None."""
-        with self._wlock:
+        with self._lock.gen_wlock():
             self._aliases = self._aliases.dissoc(alias)
 
     def intern(self, sym: sym.Symbol, var: Var, force: bool = False) -> Var:
         """Intern the Var given in this namespace mapped by the given Symbol.
         If the Symbol already maps to a Var, this method _will not overwrite_
         the existing Var mapping unless the force keyword argument is given
         and is True."""
-        with self._wlock:
+        with self._lock.gen_wlock():
             old_var = self._interns.val_at(sym, None)
             if old_var is None or force:
                 self._interns = self._interns.assoc(sym, var)
             return self._interns.val_at(sym)
 
     def unmap(self, sym: sym.Symbol) -> None:
-        with self._wlock:
+        with self._lock.gen_wlock():
             self._interns = self._interns.dissoc(sym)
 
     def find(self, sym: sym.Symbol) -> Optional[Var]:
         """Find Vars mapped by the given Symbol input or None if no Vars are
         mapped by that Symbol."""
-        with self._rlock:
+        with self._lock.gen_rlock():
             v = self._interns.val_at(sym, None)
             if v is None:
                 return self._refers.val_at(sym, None)
@@ -708,7 +702,7 @@ def find(self, sym: sym.Symbol) -> Optional[Var]:
     def add_import(self, sym: sym.Symbol, module: Module, *aliases: sym.Symbol) -> None:
         """Add the Symbol as an imported Symbol in this Namespace. If aliases are given,
         the aliases will be applied to the"""
-        with self._wlock:
+        with self._lock.gen_wlock():
             self._imports = self._imports.assoc(sym, module)
             if aliases:
                 m = self._import_aliases
@@ -722,7 +716,7 @@ def get_import(self, sym: sym.Symbol) -> Optional[BasilispModule]:
 
         First try to resolve a module directly with the given name. If no module
         can be resolved, attempt to resolve the module using import aliases."""
-        with self._rlock:
+        with self._lock.gen_rlock():
             mod = self._imports.val_at(sym, None)
             if mod is None:
                 alias = self._import_aliases.get(sym, None)
@@ -734,17 +728,17 @@ def get_import(self, sym: sym.Symbol) -> Optional[BasilispModule]:
     def add_refer(self, sym: sym.Symbol, var: Var) -> None:
         """Refer var in this namespace under the name sym."""
         if not var.is_private:
-            with self._wlock:
+            with self._lock.gen_wlock():
                 self._refers = self._refers.assoc(sym, var)
 
     def get_refer(self, sym: sym.Symbol) -> Optional[Var]:
         """Get the Var referred by Symbol or None if it does not exist."""
-        with self._rlock:
+        with self._lock.gen_rlock():
             return self._refers.val_at(sym, None)
 
     def refer_all(self, other_ns: "Namespace") -> None:
         """Refer all the Vars in the other namespace."""
-        with self._wlock:
+        with self._lock.gen_wlock():
             final_refers = self._refers
             for s, var in other_ns.interns.items():
                 if not var.is_private: