Fix and refactor the controlling thread ownership logic by ogrisel · Pull Request #290 · joblib/threadpoolctl

ogrisel · 2026-10-30T17:18:45Z

I used an LLM to review the code and it found problems with:

leaking ownership problems: calling threadpool_limits(limits=None) could unintentally release thread ownerships of pass calls.
numpy and scipy can import their own OpenBLAS copies, each with independent process-wide shared thread pool instances that are independently controllable by threadpool_limits. The existing ownership model cannot deal with this case cleanly.

This PR is a draft to refactor the ownership model to deal with those cases more robustly. See the tests for more details about the bugs fixed by this PR.

I am iterating with my agent to try to strike a good balance between robustness to edge cases and code complexity. I am not satisfied yet but I realized that opening this PR early could help in case @itamarst @cakedev0 want to provide general feedback about this whole endaevor.

A later context manager cleared control taken by a plain threadpool_limits() call or a direct set_num_threads(), and every OpenBLAS copy shared one controlling thread. Co-authored-by: Cursor <cursoragent@cursor.com>

…them. Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

…sticking. Also add SHARED_POOL_CONTROL_OWNERSHIP.increment() and decrement() without a key to act on every shared pool this thread can control, and rename the ownership helpers so they are not OpenBLAS-specific. Co-authored-by: Cursor <cursoragent@cursor.com>

Put _SharedPoolState above the registry and document the lock order. Use one helper to find claimable shared pools for both the bare increment() and the limiter. Drop a duplicate test stand-in. Co-authored-by: Cursor <cursoragent@cursor.com>

Drop _shared_pool_key(): the library filepath already identifies the pool. SHARED_POOL_CONTROL_OWNERSHIP.increment() and decrement() take an optional controller and default to get_cached_controller(). Rename variables and tests to match what they now describe. Co-authored-by: Cursor <cursoragent@cursor.com>

…rol. Only increment() and decrement() are part of SHARED_POOL_CONTROL_OWNERSHIP. Update the README note on OpenBLAS pthreads/Windows to describe one controlling thread per loaded copy. Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

OpenBLAS reports 0 threads until a BLAS call has picked a count, and set_num_threads(0) cannot return to that state. The autouse fixture now sets the processor count when the count is unset, before and after each test, and clears the controlling thread. Co-authored-by: Cursor <cursoragent@cursor.com>

…OWNERSHIP Restore the name used before the per-library ownership change. Co-authored-by: Cursor <cursoragent@cursor.com>

Revert the per-copy wording in that section and the clear_controlling_thread() call in the restore example, so the section stays focused on the limits. Co-authored-by: Cursor <cursoragent@cursor.com>

…mp_openblas The helper only returns non-OpenMP OpenBLAS libraries, so the name says so. Co-authored-by: Cursor <cursoragent@cursor.com>

clear_controlling_thread() is now an instance method that drops the claim on one loaded copy. The former class method is clear_all_controlling_threads() and still clears every copy of that controller class. Forced clears bump a generation per filepath, so clearing one copy does not drop another copy's counts. Co-authored-by: Cursor <cursoragent@cursor.com>

Drop _snapshot_shared_pool_states(), which had no callers. Clearing a holder and its sticky flag now goes through _clear_shared_pool_holder() for both per-library and per-class clears. A per-library clear no longer syncs every pool before forgetting its own count. Co-authored-by: Cursor <cursoragent@cursor.com>

…claim A threadpool_limits.wrap() decorator reused one limiter for every call, so a recursive or concurrent call overwrote the claims of the outer call and left the OpenBLAS controlling thread set after the outermost call returned. Each call now gets a fresh limiter. Also rename the ownership helpers and class to match CONTROLLING_THREAD_OWNERSHIP, make the ownership counters per-thread state set in __init__, merge the duplicated holder-clearing code, and document that a forced clear during an active scope can make the next claim sticky. Co-authored-by: Cursor <cursoragent@cursor.com>

…lder A generation epoch lets a thread skip the generation walk when nothing has been force-cleared. A limiter scope keeps the pool state it counted so dropping an unheld claim does not take the registry lock. The wrap decorator now copies itself per call instead of relying on ContextDecorator. Co-authored-by: Cursor <cursoragent@cursor.com>

…copes A limiter that lost the claim race dropped its count after an unlocked read of the winner. On free-threaded CPython that read could miss the winning write and keep a count for a pool it does not hold, making the next claim sticky. The read now takes the pool lock. The iterator also yields the pool state it already looked up, and the decrement no longer takes a flag, since the generation epoch makes the extra sync cheap. Co-authored-by: Cursor <cursoragent@cursor.com>

…braries The autouse fixture builds a ThreadpoolController, which warns when LLVM and Intel OpenMP are both loaded. pytest runs with -W error, so that warning aborted test_libomp_libiomp_warning during setup, before the test could record it. Co-authored-by: Cursor <cursoragent@cursor.com>

itamarst · 2026-10-30T18:57:30Z

Are those real issues? Respectively:

I would expect usage patterns of actual users to mostly involve effective control from one main thread, blank or not (either the only thread, or the one that starts the top-level Python thread pool).
Would you really expect a case where two different threads are interacting, one only with numpy.lingalg, one only with scipy? Having the one controlling thread seems like the most likely usage pattern as well.

Tests that hand the claim to another thread now record and assert which thread holds it, not only the thread count. The recursive-wrap test now runs through threadpool_limits.wrap on a real OpenBLAS library. Co-authored-by: Cursor <cursoragent@cursor.com>

Say loaded OpenBLAS library instead of copy, and note that NumPy and SciPy can each load their own. Describe the automatic release for context managers and decorators and point to the OpenBLASController docstring for details. Co-authored-by: Cursor <cursoragent@cursor.com>

itamarst · 2026-10-30T20:13:09Z

This seems complicated, just glancing at the number of lines of code in threadpoolctl.py specifically. If you do think this is necessary, would keying the controlling thread on filepath do the trick? That's a smaller change and allows distinguishing between different OpenBLAS instances.

EDIT: i.e. add some sort of mapping from filepath to controlling thread to the original implementation's code.

ogrisel · 2026-11-02T09:14:57Z

Yes I agree this is complicated and I am still not sure if it is worth it or no myself.

EDIT: i.e. add some sort of mapping from filepath to controlling thread to the original implementation's code.

This will solve the second bug described in this PR's decription but would probably be enough to solver the first problem. In particular, I doubt it would make test_plain_threadpool_limits_claim_survives_later_scopes pass.

I'd like to iterate to see if we can simplify this PR to reach a better complexity/robustness tradeoff but I will probably not have the time to do it this week.

That being said, I don't expect the public ownership/scope handling API to change so progress on joblib/joblib#1871 can happen independently.

itamarst · 2026-11-02T12:50:39Z

For limits=None one could just not bothering setting any limits?

EDIT: Which is in fact what the current logic does, so I guess I should go look at the failing test.

itamarst · 2026-11-02T13:03:18Z

Oh, so, for the first problem where plain limit(limits=1) ownership gets wiped by a
with block... maybe just some documentation?

Since all this does is revert to the way things were before, in older versions of threadpoolctl. Which was maybe not ideal but also there were no significant complaints?

ogrisel and others added 18 commits October 30, 2026 11:37

FIX Do not drop an OpenBLAS claim a limiter scope did not take.

e2caf0c

A later context manager cleared control taken by a plain threadpool_limits() call or a direct set_num_threads(), and every OpenBLAS copy shared one controlling thread. Co-authored-by: Cursor <cursoragent@cursor.com>

MNT Keep shared-pool claims scoped to the controller class that owns …

4a7d126

…them. Co-authored-by: Cursor <cursoragent@cursor.com>

MNT Move shared-pool claim handling onto a reusable mixin.

2457283

Co-authored-by: Cursor <cursoragent@cursor.com>

DOC Describe the manual shared-pool scope API in the changelog.

38a6ebf

Co-authored-by: Cursor <cursoragent@cursor.com>

MNT Rename the shared-pool ownership singleton to CONTROLLING_THREAD_…

67d7278

…OWNERSHIP Restore the name used before the per-library ownership change. Co-authored-by: Cursor <cursoragent@cursor.com>

DOC Keep the OpenBLAS pthreads/Windows README section unchanged

e2b0600

Revert the per-copy wording in that section and the clear_controlling_thread() call in the restore example, so the section stays focused on the limits. Co-authored-by: Cursor <cursoragent@cursor.com>

MNT Rename _require_resizable_openblas to _require_resizable_non_open…

13f0758

…mp_openblas The helper only returns non-OpenMP OpenBLAS libraries, so the name says so. Co-authored-by: Cursor <cursoragent@cursor.com>

ogrisel and others added 2 commits October 30, 2026 20:04

ogrisel commented Oct 30, 2026

View reviewed changes

Comment thread CHANGES.md Resolved

Apply suggestion from @ogrisel

52cac5c

itamarst mentioned this pull request Nov 2, 2026

Follow-up fixes for OpenBLAS-pthreads controlling thread feature #292

Open

3 tasks done

Navigation Menu

Repository navigation

Fix and refactor the controlling thread ownership logic - #290

Fix and refactor the controlling thread ownership logic#290
ogrisel wants to merge 21 commits into
joblib:masterfrom
ogrisel:fix-openblas-ownership-leak

ogrisel commented Oct 30, 2026

Uh oh!

itamarst commented Oct 30, 2026 •

edited

Loading

Uh oh!

Uh oh!

itamarst commented Oct 30, 2026 •

edited

Loading

Uh oh!

ogrisel commented Nov 2, 2026 •

edited

Loading

Uh oh!

itamarst commented Nov 2, 2026 •

edited

Loading

Uh oh!

itamarst commented Nov 2, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ogrisel commented Oct 30, 2026

Uh oh!

itamarst commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

itamarst commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel commented Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itamarst commented Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itamarst commented Nov 2, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

itamarst commented Oct 30, 2026 •

edited

Loading

itamarst commented Oct 30, 2026 •

edited

Loading

ogrisel commented Nov 2, 2026 •

edited

Loading

itamarst commented Nov 2, 2026 •

edited

Loading