Thread safety

Author: crusaderky

.github/workflows/pytest.yml

@@ -35,9 +35,9 @@ jobs:
           - mindeps
           - py310
           - py314
-        # include:
-        #   - os: ubuntu-latest
-        #     environment: nogil
+        include:
+          - os: ubuntu-latest
+            environment: nogil
 
     steps:
       - name: Checkout
@@ -57,9 +57,9 @@ jobs:
         if: matrix.environment != 'smoke'
         run: pixi run -e ${{ matrix.environment }} coverage
 
-      # - name: Free-threading stress test
-      #   if: matrix.environment == 'nogil'
-      #   run: pixi run -e nogil tests --parallel-threads=4
+      - name: Free-threading stress test
+        if: matrix.environment == 'nogil'
+        run: pixi run -e nogil tests --parallel-threads=4
 
       - name: codecov.io
         if: matrix.environment != 'smoke'

doc/index.rst

@@ -73,6 +73,13 @@ API Reference
    api/search
 
 
+Thread safety
+-------------
+pshell is fully thread safe and compatible with free-threading/noGIL interpreters.
+If thread safety is required, it is strongly recommended to use Python 3.14 or later;
+pshell does not work around bugs in earlier Python versions.
+
+
 Credits
 -------
 pshell was initially developed internally since 2014 as ``landg.bash`` by

doc/whats-new.rst

@@ -7,6 +7,10 @@ What's New
 
 v1.5.0 (unreleased)
 -------------------
+- Audited thread safety of the whole codebase
+- Fixed threading race conditions in :func:`source` and :func:`symlink`
+- Fixed issue where :func:`remove` would fail to delete directories even with
+  ``ignore_readonly=True``
 - Added formal support for Python 3.13 and 3.14 (the previous version works fine though)
 - Bumped minimum version of psutil from 5.6 to 5.7
 - Handle deprecation in psutil 6

pshell/env.py

@@ -18,7 +18,7 @@
 def source(bash_file: str | Path, *, stderr: IO | None = None) -> None:
     """Emulate the bash command ``source <bash_file>``.
     The stdout of the command, if any, will be redirected to stderr.
-    The acquired variables are injected into ``os.environment`` and are
+    The acquired variables are injected into ``os.environ`` and are
     exposed to any subprocess invoked afterwards.
 
     .. note::
@@ -41,12 +41,25 @@ def source(bash_file: str | Path, *, stderr: IO | None = None) -> None:
     """
     log.info("Sourcing environment variables from %s", bash_file)
 
-    stdout = check_output(f'source "{bash_file}" 1>&2 && env', stderr=stderr)
+    # Thread safety: spawn a bash subprocess, make it sample the previous
+    # environment, run the script and sample again.
+    delim = "!!!pshell-source-delimiter!!!"
+    stdout = check_output(
+        f'env && echo {delim} && source "{bash_file}" 1>&2 && env', stderr=stderr
+    )
 
+    is_prev = True
+    prev_env = {}
     for line in stdout.splitlines():
+        if line == delim:
+            is_prev = False
+            continue
         (key, _, value) = line.partition("=")
-
-        if key not in ("_", "", "SHLVL") and os.getenv(key) != value:
+        if key in ("_", "", "SHLVL"):
+            continue
+        if is_prev:
+            prev_env[key] = value
+        elif prev_env.get(key) != value:
             log.debug("Setting environment variable: %s=%s", key, value)
             os.environ[key] = value
 

pshell/file.py

@@ -6,7 +6,6 @@
 import errno
 import os
 import shutil
-import stat
 import sys
 from collections.abc import Callable, Iterator
 from contextlib import contextmanager
@@ -87,9 +86,10 @@ def onexc(function: object, path: str, excinfo: object) -> None:  # noqa:ARG001
                     nonlocal has_errors
                     has_errors = True
                     try:
-                        # chmod u+w
+                        # chmod u+rwx
+                        # Note that directories need the 'x' bit!
                         mode = os.stat(path).st_mode
-                        os.chmod(path, mode | stat.S_IWUSR)
+                        os.chmod(path, mode | 0o700)
                     except OSError:  # pragma: nocover
                         pass
 
@@ -130,7 +130,13 @@ def onexc(function: object, path: str, excinfo: object) -> None:  # noqa:ARG001
 
 
 def chdir(path: str | Path) -> None:
-    """Move the present-working directory (pwd) into the target directory."""
+    """Move the present-working directory (pwd) into the target directory.
+
+    .. note::
+
+       This function changes the pwd for the entire process, and as such it
+       is thread-unsafe and not context-aware.
+    """
     path = Path(path)
     log.info("chdir %s", path)
     os.chdir(resolve_env(path))
@@ -151,6 +157,13 @@ def pushd(path: str | Path) -> Iterator[None]:
         pushd mydir
         ...
         popd
+
+
+    .. note::
+
+       This function changes the pwd for the entire process, not just the
+       code inside the with block, and as such it is thread-unsafe and not
+       context-aware.
     """
     path = Path(path)
     cwd = os.getcwd()
@@ -332,18 +345,13 @@ def symlink(
 
     log.info("Creating symlink %s => %s", src, dst)
 
-    if abspath:
-        os.symlink(real_src, real_dst)
-    else:
-        cwd_backup = os.getcwd()
-        os.chdir(os.path.realpath(os.path.dirname(real_dst)))
-        try:
-            # Generate shortest possible relative path
-            real_src = os.path.relpath(os.path.realpath(real_src))
-            real_dst = os.path.relpath(os.path.realpath(real_dst))
-            os.symlink(real_src, real_dst)
-        finally:
-            os.chdir(cwd_backup)
+    if not abspath:
+        # Generate shortest possible relative path
+        real_src = os.path.relpath(
+            os.path.realpath(real_src),
+            start=os.path.realpath(os.path.dirname(real_src)),
+        )
+    os.symlink(real_src, real_dst)
 
 
 def exists(path: str | Path) -> bool:

pshell/tests/__init__.py

@@ -1,4 +1,5 @@
 import os
+import uuid
 
 import pytest
 
@@ -13,3 +14,10 @@ class StubError(Exception):
 
 unix_only = pytest.mark.skipif(os.name == "nt", reason="Unix only")
 windows_only = pytest.mark.skipif(os.name != "nt", reason="Windows only")
+
+
+def get_name(suffix: str = "") -> str:
+    """A unique name to be used with pytest-run-parallel"""
+    suffix = f"_{suffix}" if suffix else ""
+    id_ = str(uuid.uuid4()).replace("-", "")
+    return f"pshell_tests{suffix}_{id_}"

pshell/tests/conftest.py

@@ -1,3 +1,5 @@
+import os
+import sys
 from pathlib import Path
 
 import pytest
@@ -13,3 +15,19 @@ def test_open(str_or_path):
             sh.open(str_or_path("foo/bar/baz"))
     """
     return request.param
+
+
+@pytest.fixture(autouse=True)
+def clear_names():
+    """Clear any environment variables created with get_name()"""
+    yield
+    for key in list(os.environ.keys()):
+        if key.startswith("pshell_tests"):
+            del os.environ[key]
+
+
+if "pytest_run_parallel" not in sys.modules:
+
+    @pytest.fixture
+    def thread_index():
+        return 0

pshell/tests/data/sleep20.bat

@@ -0,0 +1,4 @@
+import time
+
+print("ready", flush=True)
+time.sleep(20)