FIX intermittent Windows module enumeration failure - #219
Conversation
Use CreateToolhelp32Snapshot for atomic module discovery on Windows, with GetModuleFileNameExW for long paths and graceful per-module fallbacks when handles go stale. Add Windows integration tests for paths longer than MAX_PATH. Co-authored-by: Cursor <cursoragent@cursor.com>
- Run repeated-init stress test on all platforms - Drop redundant OpenBLAS glob pattern and win32 guard in utils - Remove MAX_PATH tests from SAFE_SKIPPED_TESTS whitelist Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Dugal Harris <dugal@leftfield.online> Co-authored-by: Cursor <cursoragent@cursor.com>
Correct numpy.libs glob to use the site-packages sibling directory, add conda-forge Library/bin patterns, and fall back to the loaded OpenBLAS filepath from ThreadpoolController when globs find nothing. Co-authored-by: Cursor <cursoragent@cursor.com>
Use extended-length paths when copying and loading test DLLs so LoadLibrary works beyond MAX_PATH, and pass the module buffer directly to EnumProcessModulesEx to match its ctypes signature. Co-authored-by: Cursor <cursoragent@cursor.com>
Use libopenblas-prefixed destination DLL names so conda builds are recognized, normalize extended-length paths for comparisons, prefer shipped libscipy_openblas sources, and detect truncation when the resolved path fills the GetModuleFileNameExW buffer. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Load cv2 before stressing ThreadpoolController() to match the Windows conda-forge repro from issue joblib#217, and install OpenCV in CI jobs that exercise this test. Co-authored-by: Cursor <cursoragent@cursor.com>
Use GetCurrentProcess(), trust snapshot szExePath on the happy path, and resolve fallback module paths with GetModuleFileNameW before the slower GetModuleFileNameExW long-path lookup. Co-authored-by: Cursor <cursoragent@cursor.com>
ogrisel
left a comment
There was a problem hiding this comment.
Code style review.
Move ThreadpoolController usage to module scope in utils, hoist conda OpenBLAS globs out of the numpy import block, and inline Windows MAX_PATH test setup in the tests instead of small utils helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
ogrisel
left a comment
There was a problem hiding this comment.
I think this LGTM. The licensing issue is still there but will be addressed in a follow-up PR to decouple the concerns.
cc @jeremiedbb @tomMoral.
|
It seems that clang conda-forge packaging has changed because it can no longer find its openmp but it's unrelated to this PR. |
|
I synced with master and everything is green thanks to @lesteve's recent CI fixes. Shall we merge? |
|
Codex [P1] Bound snapshot retries so long paths reach the fallbackFile: When Limit the retries and raise after exhaustion so discovery can use the long-path-capable fallback. |
|
I can reproduce the original on my Windows VM so I'll give it a try as a sanity check. |
|
Here is the reply from agent to your agent: The Codex diagnosis is incorrect, but a retry cap would still be a reasonable hardening change.
Long DLL paths are a different failure mode:
A bounded retry (say 8-16 attempts, no sleep) then A related but separate gap: if the snapshot succeeds with a truncated |
Keep the Toolhelp snapshot as the module list, and only call GetModuleFileNameExW for empty or MAX_PATH-truncated szExePath values instead of skipping those libraries. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@copilot resolve the merge conflicts in this pull request |
Resolve conflicts with joblib#243 by keeping Windows on the Toolhelp snapshot enumerator. ctypes.util.dllist still uses EnumProcessModules, which is the race this PR fixes. Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
…test Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
|
I synced with So this means that the ugly extra complexity introduced in that PR to get those non-regression test to also pass on older CPython version will go away in a few years once we stop support CPython < 3.14. I think this PR deserves to be merged, at least to introduce the non-regression and stress tests. If we don't want to introduce the extra complexity, we could revert it and skip the new tests on older Python versions. WDYT @lesteve? |
|
Actually, my above comment is wrong. The custom win32 code branch is always used, even on newer CPython versions. Let's check if this is needed or not. |
Prefer the stdlib enumerator when it exists. On Windows, fall back to the Toolhelp snapshot path if dllist raises OSError from concurrent module load/unload. Skip the internal MAX_PATH limit test when dllist bypasses that limit. Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Use dllist alone on Python 3.14+ so we can see whether the snapshot path is still needed as an OSError recovery. Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
|
Let's see if the new tests fail with dlllist on windows. |
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
|
This is great: the dllist method passes all the non-regression stress tests on Windows with recent versions of Python. This means that once we drop older Python support we will be able to significantly trim the windows specific code path, including the extra complexity introduced in this PR. |
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Set GetModuleFileNameExW.restype to DWORD, warn when both filename APIs fail, and unwrap the snapshot-first docstring line. Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
…lback If dllist raises OSError, emit a RuntimeWarning with the exception so the race can be reported upstream to CPython with a minimal reproducer. Do not fall back to the Windows snapshot enumerator. Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
ogrisel
left a comment
There was a problem hiding this comment.
LGTM. The CI is really stable, even when running the stress test on more cases.
Fix #217
Summary
OSError("GetModuleFileNameEx failed")on Windows when DLLs are loaded or unloaded during library discovery (reported with conda-forge OpenCV).EnumProcessModulesEx+GetModuleFileNameExWscan with a snapshot-first approach usingCreateToolhelp32Snapshot, keeping long-path support viaGetModuleFileNameExWand graceful per-module fallbacks.MAX_PATH, inspired by the local repro documented in ENH Support path length > MAX_PATH=260 on Windows #189 (comment).Closes #217
Test plan
py311_conda_forge_openblas): new teststest_windows_library_path_longer_than_max_pathandtest_windows_library_path_exceeds_internal_limitpassAI assistance disclosure
This pull request was prepared with assistance from an AI coding agent (Cursor / Claude).
Made with Cursor