diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 50238058..de058441 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -51,22 +51,26 @@ jobs: PYTHON_VERSION: "*" PACKAGER: "conda-forge" BLAS: "mkl" + INSTALL_OPENCV: "true" # Windows env with numpy, scipy, OpenBLAS installed through conda-forge - name: py311_conda_forge_openblas os: windows-latest PYTHON_VERSION: "3.11" PACKAGER: "conda-forge" BLAS: "openblas" + INSTALL_OPENCV: "true" # Windows env with numpy, scipy installed through conda - name: py310_conda os: windows-latest PYTHON_VERSION: "3.10" PACKAGER: "conda" + INSTALL_OPENCV: "true" # Windows env with numpy, scipy installed through pip - name: py39_pip os: windows-latest PYTHON_VERSION: "3.9" PACKAGER: "pip" + INSTALL_OPENCV: "true" # MacOS env with OpenMP installed through homebrew - name: py39_conda_homebrew_libomp @@ -157,6 +161,7 @@ jobs: PYTHON_VERSION: "*" BLAS: "openblas" OPENBLAS_THREADING_LAYER: "openmp" + INSTALL_OPENCV: "true" CC_OUTER_LOOP: "gcc" CC_INNER_LOOP: "gcc" # Linux environment with no numpy and heterogeneous OpenMP runtimes. diff --git a/CHANGES.md b/CHANGES.md index 65d523e2..c2351838 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,12 @@ 3.7.0 (TBD) =========== +- Fixed an intermittent `OSError` on Windows when DLLs are loaded or unloaded + concurrently during library discovery (for example when importing conda-forge + OpenCV). Windows module enumeration now uses a snapshot-first approach with + graceful per-module fallbacks. + https://github.com/joblib/threadpoolctl/pull/219 + - Only warn about simultaneous `libomp` and `libiomp` usage on Linux, where the incompatibility is known to cause crashes. diff --git a/continuous_integration/install.sh b/continuous_integration/install.sh index 8e6523fc..accf6fea 100644 --- a/continuous_integration/install.sh +++ b/continuous_integration/install.sh @@ -65,6 +65,9 @@ if [[ "$PACKAGER" == "conda" ]]; then if [[ -n "$BLAS" ]]; then TO_INSTALL="$TO_INSTALL blas=*=$BLAS" fi + fi + if [[ "$INSTALL_OPENCV" == "true" ]]; then + TO_INSTALL="$TO_INSTALL opencv" fi make_conda "defaults" "$TO_INSTALL" @@ -73,6 +76,9 @@ elif [[ "$PACKAGER" == "conda-forge" ]]; then if [[ "$BLAS" == "openblas" && "$OPENBLAS_THREADING_LAYER" == "openmp" ]]; then TO_INSTALL="$TO_INSTALL libopenblas=*=*openmp*" fi + if [[ "$INSTALL_OPENCV" == "true" ]]; then + TO_INSTALL="$TO_INSTALL opencv" + fi make_conda "conda-forge" "$TO_INSTALL" elif [[ "$PACKAGER" == "pip" ]]; then @@ -82,6 +88,9 @@ elif [[ "$PACKAGER" == "pip" ]]; then if [[ "$NO_NUMPY" != "true" ]]; then pip install numpy scipy fi + if [[ "$INSTALL_OPENCV" == "true" ]]; then + pip install opencv-python + fi elif [[ "$PACKAGER" == "pip-dev" ]]; then # Use conda to build an empty python env and then use pip to install diff --git a/tests/test_threadpoolctl.py b/tests/test_threadpoolctl.py index bedca67e..9cc324d4 100644 --- a/tests/test_threadpoolctl.py +++ b/tests/test_threadpoolctl.py @@ -4,7 +4,10 @@ import re import subprocess import sys +import ctypes +import shutil +import threadpoolctl from threadpoolctl import threadpool_limits, threadpool_info from threadpoolctl import ThreadpoolController from threadpoolctl import _ALL_PREFIXES, _ALL_USER_APIS @@ -12,6 +15,8 @@ from .utils import cython_extensions_compiled from .utils import check_nested_prange_blas from .utils import libopenblas_paths +from .utils import get_openblas_dll_path +from .utils import make_long_windows_path from .utils import scipy from .utils import threadpool_info_from_subprocess from .utils import select @@ -793,3 +798,108 @@ def test_custom_controller(): assert mylib_controller.num_threads == 1 assert ThreadpoolController().info() == original_info + + +def test_threadpool_controller_repeated_init(): + """Stress-test repeated library discovery. + + Non-regression test for a Windows-specific problem where DLLs loaded or + unloaded concurrently during discovery could raise an OSError. The failure + was originally reproduced after importing OpenCV on Windows conda-forge; + see https://github.com/joblib/threadpoolctl/issues/217 + """ + pytest.importorskip("cv2") + + for _ in range(100): + ThreadpoolController() + + +@pytest.mark.skipif(sys.platform != "win32", reason="Windows-only test") +def test_windows_library_path_longer_than_max_path(tmp_path): + """OpenBLAS loaded from a path longer than MAX_PATH is discovered. + + Regression test inspired by the local repro in + https://github.com/joblib/threadpoolctl/pull/189#issuecomment-2714235916 + """ + src_dll = get_openblas_dll_path() + if src_dll is None: + pytest.skip("Requires OpenBLAS on Windows") + + long_path = make_long_windows_path( + tmp_path, "libopenblas_long_path_test.dll", min_length=261 + ) + extended_path = os.path.abspath(str(long_path)) + if not extended_path.startswith("\\\\?\\"): + if extended_path.startswith("\\\\"): + extended_path = "\\\\?\\UNC\\" + extended_path[2:] + else: + extended_path = "\\\\?\\" + extended_path + shutil.copy2(src_dll, extended_path) + ctypes.CDLL(extended_path) + + expected_path = os.path.abspath(str(long_path)) + if expected_path.startswith("\\\\?\\"): + expected_path = expected_path[4:] + if expected_path.startswith("UNC\\"): + expected_path = "\\\\" + expected_path[4:] + expected_path = os.path.normcase(os.path.normpath(expected_path)) + openblas_info = ThreadpoolController().select(internal_api="openblas").info() + + long_path_entries = [info for info in openblas_info if len(info["filepath"]) > 260] + assert len(long_path_entries) >= 1 + normalized_filepaths = [] + for info in long_path_entries: + filepath = info["filepath"] + if filepath.startswith("\\\\?\\"): + filepath = filepath[4:] + if filepath.startswith("UNC\\"): + filepath = "\\\\" + filepath[4:] + normalized_filepaths.append(os.path.normcase(os.path.normpath(filepath))) + assert expected_path in normalized_filepaths + + +@pytest.mark.skipif(sys.platform != "win32", reason="Windows-only test") +def test_windows_library_path_exceeds_internal_limit(tmp_path, monkeypatch): + """Libraries with a path longer than the internal limit are ignored. + + Regression test inspired by the local repro in + https://github.com/joblib/threadpoolctl/pull/189#issuecomment-2714235916 + """ + src_dll = get_openblas_dll_path() + if src_dll is None: + pytest.skip("Requires OpenBLAS on Windows") + + monkeypatch.setattr(threadpoolctl, "_WINDOWS_MAX_LIBRARY_PATH_LENGTH", 300) + + long_path = make_long_windows_path( + tmp_path, "libopenblas_path_too_long.dll", min_length=400 + ) + extended_path = os.path.abspath(str(long_path)) + if not extended_path.startswith("\\\\?\\"): + if extended_path.startswith("\\\\"): + extended_path = "\\\\?\\UNC\\" + extended_path[2:] + else: + extended_path = "\\\\?\\" + extended_path + shutil.copy2(src_dll, extended_path) + ctypes.CDLL(extended_path) + + expected_path = os.path.abspath(str(long_path)) + if expected_path.startswith("\\\\?\\"): + expected_path = expected_path[4:] + if expected_path.startswith("UNC\\"): + expected_path = "\\\\" + expected_path[4:] + expected_path = os.path.normcase(os.path.normpath(expected_path)) + with pytest.warns(RuntimeWarning, match="path too long"): + info = ThreadpoolController().info() + + filepaths = set() + for entry in info: + if "filepath" not in entry: + continue + filepath = entry["filepath"] + if filepath.startswith("\\\\?\\"): + filepath = filepath[4:] + if filepath.startswith("UNC\\"): + filepath = "\\\\" + filepath[4:] + filepaths.add(os.path.normcase(os.path.normpath(filepath))) + assert expected_path not in filepaths diff --git a/tests/utils.py b/tests/utils.py index 86fe1b0b..0c8d747a 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,9 +1,10 @@ -import os import json +import os import sys import threadpoolctl from glob import glob from os.path import dirname, normpath +from pathlib import Path from subprocess import check_output # Path to shipped openblas for libraries such as numpy or scipy @@ -17,9 +18,21 @@ np.dot(np.ones(1000), np.ones(1000)) libopenblas_patterns.append(os.path.join(np.__path__[0], ".libs", "libopenblas*")) + numpy_site_packages = os.path.dirname(np.__path__[0]) + libopenblas_patterns.append( + os.path.join(numpy_site_packages, "numpy.libs", "libscipy_openblas*.dll") + ) except ImportError: pass +if sys.platform == "win32": + libopenblas_patterns.append( + os.path.join(sys.prefix, "Library", "bin", "openblas*.dll") + ) + libopenblas_patterns.append( + os.path.join(sys.prefix, "Library", "bin", "libopenblas*.dll") + ) + try: import scipy @@ -30,6 +43,10 @@ libopenblas_patterns.append( os.path.join(scipy.__path__[0], ".libs", "libopenblas*") ) + scipy_site_packages = os.path.dirname(scipy.__path__[0]) + libopenblas_patterns.append( + os.path.join(scipy_site_packages, "scipy.libs", "libscipy_openblas*.dll") + ) except ImportError: scipy = None @@ -86,3 +103,45 @@ def select(info, **kwargs): ] return selected_info + + +def get_openblas_dll_path(): + """Return a path to an OpenBLAS DLL that can be copied for Windows tests.""" + if libopenblas_paths: + return sorted( + libopenblas_paths, + key=lambda path: ( + 0 + if "libscipy_openblas" in os.path.basename(path).lower() + else 1 if "libopenblas" in os.path.basename(path).lower() else 2 + ), + )[0] + + controllers = ( + threadpoolctl.ThreadpoolController() + .select(internal_api="openblas") + .lib_controllers + ) + if not controllers: + return None + + filepath = controllers[0].filepath + if os.path.isfile(filepath): + return filepath + return None + + +def make_long_windows_path(base_dir, filename, min_length=261): + """Nest padded directories under base_dir until base/.../filename >= min_length.""" + padding_segments = ["a" * 100, "b" * 100, "c" * 100, "d" * 100] + current = Path(base_dir) + segment_index = 0 + target = current / filename + + while len(str(target)) < min_length: + current = current / padding_segments[segment_index % len(padding_segments)] + segment_index += 1 + target = current / filename + + target.parent.mkdir(parents=True, exist_ok=True) + return target diff --git a/threadpoolctl.py b/threadpoolctl.py index 155b14bf..662dd847 100644 --- a/threadpoolctl.py +++ b/threadpoolctl.py @@ -47,6 +47,10 @@ # disable it while under the scope of the outer OpenMP parallel section. os.environ.setdefault("KMP_DUPLICATE_LIB_OK", "True") +# Hard limit for Windows library paths resolved through GetModuleFileNameExW. +# Kept below unlimited for security reasons; see CHANGES.md and PR #189. +_WINDOWS_MAX_LIBRARY_PATH_LENGTH = 2600 + # Structure to cast the info on dynamically loaded library. See # https://linux.die.net/man/3/dl_iterate_phdr for more details. _SYSTEM_UINT = ctypes.c_uint64 if sys.maxsize > 2**32 else ctypes.c_uint32 @@ -980,7 +984,7 @@ def _load_libraries(self): if sys.platform == "darwin": self._find_libraries_with_dyld() elif sys.platform == "win32": - self._find_libraries_with_enum_process_module_ex() + self._find_libraries_on_windows() elif "pyodide" in sys.modules: self._find_libraries_pyodide() else: @@ -1050,79 +1054,241 @@ def _find_libraries_with_dyld(self): # Store the library controller if it is supported and selected self._make_controller_from_path(filepath) - def _find_libraries_with_enum_process_module_ex(self): + def _find_libraries_on_windows(self): """Loop through loaded libraries and return binders on supported ones This function is expected to work on windows system only. - This code is adapted from code by Philipp Hagemeister @phihag available - at https://stackoverflow.com/questions/17474574 + + Module discovery uses a snapshot-first strategy: when + ``CreateToolhelp32Snapshot`` succeeds, ``szExePath`` values are already + complete and shorter than ``MAX_PATH``. When the snapshot fails (for + example because a loaded DLL lives on a long path), enumeration falls + back to ``EnumProcessModulesEx`` and resolves paths with + ``GetModuleFileNameExW`` using a larger buffer. The snapshot based + approach is more robust in case of concurrent DLL loading/unloading, + however it cannot handle long paths, hence the need for the fallback. """ - from ctypes.wintypes import DWORD, HMODULE, MAX_PATH + ps_api = self._get_windll("Psapi") + kernel_32 = self._get_windll("kernel32") + self._setup_windows_module_apis(ps_api, kernel_32) - PROCESS_QUERY_INFORMATION = 0x0400 - PROCESS_VM_READ = 0x0010 + h_process = kernel_32.GetCurrentProcess() - LIST_LIBRARIES_ALL = 0x03 + # Allocate a buffer for long path names; see _WINDOWS_MAX_LIBRARY_PATH_LENGTH. + max_path = _WINDOWS_MAX_LIBRARY_PATH_LENGTH + path_buf = ctypes.create_unicode_buffer(max_path) - ps_api = self._get_windll("Psapi") - kernel_32 = self._get_windll("kernel32") + try: + modules = self._snapshot_loaded_modules(kernel_32) + except OSError: + modules = None + + if modules is not None: + for _h_module, snapshot_path in modules: + filepath = self._snapshot_module_filepath(snapshot_path) + if filepath is not None: + self._make_controller_from_path(filepath) + else: + self._find_libraries_with_enum_process_modules_ex( + ps_api, kernel_32, h_process, max_path, path_buf + ) - h_process = kernel_32.OpenProcess( - PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, False, os.getpid() - ) - if not h_process: # pragma: no cover - raise OSError(f"Could not open PID {os.getpid()}") + @classmethod + def _setup_windows_module_apis(cls, ps_api, kernel_32): + """Set ctypes signatures for Windows module enumeration APIs.""" + from ctypes.wintypes import BOOL, DWORD, HANDLE, HMODULE + + if getattr(cls, "_windows_module_apis_configured", False): + return + + kernel_32.GetCurrentProcess.restype = HANDLE + kernel_32.CreateToolhelp32Snapshot.argtypes = [DWORD, DWORD] + kernel_32.CreateToolhelp32Snapshot.restype = HANDLE + kernel_32.Module32FirstW.argtypes = [HANDLE, ctypes.c_void_p] + kernel_32.Module32FirstW.restype = BOOL + kernel_32.Module32NextW.argtypes = [HANDLE, ctypes.c_void_p] + kernel_32.Module32NextW.restype = BOOL + kernel_32.GetModuleFileNameW.argtypes = [HMODULE, ctypes.c_wchar_p, DWORD] + kernel_32.GetModuleFileNameW.restype = DWORD + kernel_32.CloseHandle.argtypes = [HANDLE] + kernel_32.CloseHandle.restype = BOOL + + ps_api.EnumProcessModulesEx.argtypes = [ + HANDLE, + ctypes.POINTER(HMODULE), + DWORD, + ctypes.POINTER(DWORD), + DWORD, + ] + ps_api.EnumProcessModulesEx.restype = BOOL + ps_api.GetModuleFileNameExW.argtypes = [ + HANDLE, + HMODULE, + ctypes.c_wchar_p, + DWORD, + ] + ps_api.GetModuleFileNameExW.restype = BOOL + + cls._windows_module_apis_configured = True + + def _snapshot_loaded_modules(self, kernel_32): + """Return loaded modules as (hModule, snapshot_path) pairs. + + Uses CreateToolhelp32Snapshot for an atomic view of loaded modules, which + is more robust than EnumProcessModulesEx when DLLs are loaded or unloaded + concurrently. + """ + from ctypes.wintypes import DWORD, HANDLE, MAX_PATH + + class MODULEENTRY32W(ctypes.Structure): + _fields_ = [ + ("dwSize", DWORD), + ("th32ModuleID", DWORD), + ("th32ProcessID", DWORD), + ("GlblcntUsage", DWORD), + ("ProccntUsage", DWORD), + ("modBaseAddr", ctypes.POINTER(ctypes.c_byte)), + ("modBaseSize", DWORD), + ("hModule", HANDLE), + ("szModule", ctypes.c_wchar * 256), + ("szExePath", ctypes.c_wchar * MAX_PATH), + ] + + TH32CS_SNAPMODULE = 0x00000008 + TH32CS_SNAPMODULE32 = 0x00000010 + ERROR_BAD_LENGTH = 0x0018 + ERROR_NO_MORE_FILES = 0x0012 + INVALID_HANDLE_VALUE = HANDLE(-1).value + + while True: + snap_handle = kernel_32.CreateToolhelp32Snapshot( + TH32CS_SNAPMODULE | TH32CS_SNAPMODULE32, os.getpid() + ) + if snap_handle == INVALID_HANDLE_VALUE: + err = ctypes.get_last_error() + if err == ERROR_BAD_LENGTH: + continue + msg = ctypes.FormatError(err).strip() + raise OSError(f"CreateToolhelp32Snapshot failed: {msg}") + break + modules = [] try: - buf_count = 256 - needed = DWORD() - # Grow the buffer until it becomes large enough to hold all the - # module headers + lib_entry = MODULEENTRY32W() + lib_entry.dwSize = ctypes.sizeof(MODULEENTRY32W) + if not kernel_32.Module32FirstW( + snap_handle, ctypes.byref(lib_entry) + ): # pragma: no cover + err = ctypes.get_last_error() + msg = ctypes.FormatError(err).strip() + raise OSError(f"Module32FirstW failed: {msg}") + while True: - buf = (HMODULE * buf_count)() - buf_size = ctypes.sizeof(buf) - if not ps_api.EnumProcessModulesEx( - h_process, - ctypes.byref(buf), - buf_size, - ctypes.byref(needed), - LIST_LIBRARIES_ALL, - ): - raise OSError("EnumProcessModulesEx failed") - if buf_size >= needed.value: + modules.append((lib_entry.hModule, lib_entry.szExePath)) + if not kernel_32.Module32NextW(snap_handle, ctypes.byref(lib_entry)): + err = ctypes.get_last_error() + if err != ERROR_NO_MORE_FILES: # pragma: no cover + msg = ctypes.FormatError(err).strip() + raise OSError(f"Module32NextW failed: {msg}") break - buf_count = needed.value // (buf_size // buf_count) - - count = needed.value // (buf_size // buf_count) - h_modules = map(HMODULE, buf[:count]) - - # Loop through all the module headers and get the library path - # Allocate a buffer for the path 10 times the size of MAX_PATH to take - # into account long path names. - max_path = 10 * MAX_PATH - buf = ctypes.create_unicode_buffer(max_path) - n_size = DWORD() - for h_module in h_modules: - # Get the path of the current module - if not ps_api.GetModuleFileNameExW( - h_process, h_module, ctypes.byref(buf), ctypes.byref(n_size) - ): - raise OSError("GetModuleFileNameEx failed") - filepath = buf.value - - if len(filepath) == max_path: # pragma: no cover - warnings.warn( - "Could not get the full path of a dynamic library (path too " - "long). This library will be ignored and threadpoolctl might " - "not be able to control or display information about all " - f"loaded libraries. Here's the truncated path: {filepath!r}", - RuntimeWarning, - ) - else: - # Store the library controller if it is supported and selected - self._make_controller_from_path(filepath) finally: - kernel_32.CloseHandle(h_process) + kernel_32.CloseHandle(snap_handle) + + return modules + + def _snapshot_module_filepath(self, snapshot_path): + """Return a snapshot module path, or None if it should be skipped.""" + from ctypes.wintypes import MAX_PATH + + if not snapshot_path: + return None + + if len(snapshot_path) >= MAX_PATH - 1: # pragma: no cover + warnings.warn( + "Could not get the full path of a dynamic library. This library " + "will be ignored and threadpoolctl might not be able to control or " + f"display information about all loaded libraries. Here's the " + f"truncated path: {snapshot_path!r}", + RuntimeWarning, + ) + return None + + return snapshot_path + + def _resolve_module_filepath( + self, + ps_api, + kernel_32, + h_process, + h_module, + max_path, + path_buf, + ): + """Return the full path for a module, or None if it should be skipped.""" + from ctypes.wintypes import MAX_PATH + + n_size = kernel_32.GetModuleFileNameW(h_module, path_buf, MAX_PATH) + if n_size and n_size < MAX_PATH - 1: + return path_buf.value + + if ps_api.GetModuleFileNameExW(h_process, h_module, path_buf, max_path): + filepath = path_buf.value + if len(filepath) >= max_path - 1: # pragma: no cover + warnings.warn( + "Could not get the full path of a dynamic library (path too " + "long). This library will be ignored and threadpoolctl might " + "not be able to control or display information about all " + f"loaded libraries. Here's the truncated path: {filepath!r}", + RuntimeWarning, + ) + return None + return filepath + + return None + + def _find_libraries_with_enum_process_modules_ex( + self, ps_api, kernel_32, h_process, max_path, path_buf + ): + """Fallback Windows module enumeration using EnumProcessModulesEx. + + This code is adapted from code by Philipp Hagemeister @phihag available + at https://stackoverflow.com/questions/17474574 + """ + from ctypes.wintypes import DWORD, HMODULE + + LIST_LIBRARIES_ALL = 0x03 + + buf_count = 256 + needed = DWORD() + # Grow the buffer until it becomes large enough to hold all the + # module headers + while True: + buf = (HMODULE * buf_count)() + buf_size = ctypes.sizeof(buf) + if not ps_api.EnumProcessModulesEx( + h_process, + buf, + buf_size, + ctypes.byref(needed), + LIST_LIBRARIES_ALL, + ): + raise OSError("EnumProcessModulesEx failed") + if buf_size >= needed.value: + break + buf_count = needed.value // (buf_size // buf_count) + + count = needed.value // (buf_size // buf_count) + for h_module in map(HMODULE, buf[:count]): + filepath = self._resolve_module_filepath( + ps_api, + kernel_32, + h_process, + h_module, + max_path=max_path, + path_buf=path_buf, + ) + if filepath is not None: + self._make_controller_from_path(filepath) def _find_libraries_pyodide(self): """Pyodide specific implementation for finding loaded libraries. @@ -1266,7 +1432,7 @@ def _get_windll(cls, dll_name): """Load a windows DLL""" dll = cls._system_libraries.get(dll_name) if dll is None: - dll = ctypes.WinDLL(f"{dll_name}.dll") + dll = ctypes.WinDLL(f"{dll_name}.dll", use_last_error=True) cls._system_libraries[dll_name] = dll return dll