From 06879e1a95f7ca8ba66a8d9fcd2b4956120b6254 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 16:16:49 +0000 Subject: [PATCH 1/2] Initial plan From 72873295e7e568a1df3bb83a028f3a3c2591f461 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 16:23:08 +0000 Subject: [PATCH 2/2] Fix thread-safety in temporary_cd by not calling os.chdir() in no-argument form Agent-Logs-Url: https://github.com/openforcefield/openff-utilities/sessions/0701659a-296a-4946-aa02-05a346cf1b14 Co-authored-by: mattwthompson <7935382+mattwthompson@users.noreply.github.com> --- openff/utilities/_tests/test_utilities.py | 57 +++++++++++++++++++++-- openff/utilities/utilities.py | 48 +++++++++++-------- 2 files changed, 83 insertions(+), 22 deletions(-) diff --git a/openff/utilities/_tests/test_utilities.py b/openff/utilities/_tests/test_utilities.py index 1cb34f6..8d42012 100644 --- a/openff/utilities/_tests/test_utilities.py +++ b/openff/utilities/_tests/test_utilities.py @@ -1,4 +1,5 @@ import os +import threading import pytest @@ -90,9 +91,12 @@ def test_temporary_cd(): assert compare_paths(os.getcwd(), original_directory) - # Move to a temporary directory - with temporary_cd(): - assert not compare_paths(os.getcwd(), original_directory) + # Move to a temporary directory: CWD is NOT changed in the no-argument form. + # The caller must use the yielded path for file operations. + with temporary_cd() as tmpdir: + assert compare_paths(os.getcwd(), original_directory) + assert os.path.isdir(tmpdir) + assert not compare_paths(tmpdir, original_directory) assert compare_paths(os.getcwd(), original_directory) @@ -108,6 +112,53 @@ def test_temporary_cd(): assert compare_paths(os.getcwd(), original_directory) +def test_temporary_cd_yields_path(): + """Tests that temporary_cd yields the absolute path of the directory.""" + + original_directory = os.getcwd() + + # Auto temp dir: yielded path should be an absolute path different from original + with temporary_cd() as tmpdir: + assert os.path.isabs(tmpdir) + assert not compare_paths(tmpdir, original_directory) + # CWD is NOT changed in the no-argument form (thread-safe behaviour) + assert compare_paths(os.getcwd(), original_directory) + + # Specific directory: yielded path should match the absolute path of the given dir + with temporary_cd(os.pardir) as tmpdir: + expected = os.path.abspath(os.path.join(original_directory, os.pardir)) + assert compare_paths(tmpdir, expected) + assert compare_paths(os.getcwd(), expected) + + # Empty string: yielded path should be the original directory + with temporary_cd("") as tmpdir: + assert compare_paths(tmpdir, original_directory) + + +def test_temporary_cd_thread_safety(): + """Tests that temporary_cd is thread-safe when using the yielded absolute path.""" + + errors: list[str] = [] + + def func(n: int) -> None: + with temporary_cd() as tmpdir: + file_path = os.path.join(tmpdir, "f.txt") + with open(file_path, "w") as f: + f.write(str(n)) + with open(file_path) as f: + read_value = int(f.read()) + if read_value != n: + errors.append(f"Expected {n} but found {read_value}") + + threads = [threading.Thread(target=func, args=(i,)) for i in range(8)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert not errors, f"Thread safety errors: {errors}" + + def test_has_package(): assert has_package("os") assert has_package("pytest") diff --git a/openff/utilities/utilities.py b/openff/utilities/utilities.py index 9fd5316..9f521b1 100644 --- a/openff/utilities/utilities.py +++ b/openff/utilities/utilities.py @@ -150,39 +150,49 @@ def _is_executable(fpath: str) -> bool: @contextmanager -def temporary_cd(directory_path: str | None = None) -> Generator[None, None, None]: +def temporary_cd(directory_path: str | None = None) -> Generator[str, None, None]: """Temporarily move the current working directory to the path specified. If no path is given, a temporary directory will be - created, moved into, and then destroyed when the context manager - is closed. + created and its path yielded; the directory is destroyed when the + context manager is closed. + + When ``directory_path`` is ``None`` (the default), ``os.chdir`` is + **not** called, making this form safe to use from multiple threads + concurrently. Use the yielded absolute path for all file + operations inside the block (e.g. + ``open(os.path.join(tmpdir, "file"))``). + + When an explicit ``directory_path`` is given, ``os.chdir`` is still + called for backward compatibility. Note that ``os.chdir`` is a + process-wide operation and is therefore **not** thread-safe. Parameters ---------- directory_path: str, optional - Returns - ------- + Yields + ------ + str + The absolute path of the directory. """ if directory_path is not None and len(directory_path) == 0: - yield + yield os.getcwd() return - old_directory = os.getcwd() + if directory_path is None: + with TemporaryDirectory() as new_directory: + yield new_directory - try: - if directory_path is None: - with TemporaryDirectory() as new_directory: - os.chdir(new_directory) - yield - - else: - os.chdir(directory_path) - yield - - finally: - os.chdir(old_directory) + else: + abs_directory = os.path.abspath(directory_path) + old_directory = os.getcwd() + try: + os.chdir(abs_directory) + yield abs_directory + finally: + os.chdir(old_directory) def get_data_dir_path(relative_path: str, package_name: str) -> str: