Skip to content

Fix two NDArray thread-safety bugs blocking free-threaded support (#555) - #713

Merged
FrancescAlted merged 5 commits into
Blosc:mainfrom
Johnny-Kao:fix/free-threaded-ndarray-read-races
Sep 24, 2026
Merged

FrancescAlted merged 5 commits into
Blosc:mainfrom
Johnny-Kao:fix/free-threaded-ndarray-read-races

Conversation

@Johnny-Kao

@Johnny-Kao Johnny-Kao commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

While investigating #555 (free-threaded/no-GIL support), I found and fixed two
thread-safety bugs in NDArray's array-level read paths that cause data
corruption or a crash under -X gil=0. These are narrow correctness fixes,
not the full free-threading enablement sketched in #555 (setting
RELEASEGIL) — in fact, testing showed that flipping RELEASEGIL alone,
without these two fixes, would surface exactly this kind of corruption/crash.

Commits

1. get_1d_span_numpy: stop sharing the SChunk's mutable dctx across threads

This method (used internally by the indexing query planner) borrowed
self.array.sc.dctx when non-NULL. That context is mutable state;
concurrent decompress calls from multiple threads can corrupt each other's
output. Changed it to always create a private per-call dctx. To avoid
regressing on schunks whose decompression depends on per-schunk state (e.g.
dictionaries resolved through dparams.schunk), the private context is
still associated with the SChunk rather than built from bare
BLOSC2_DPARAMS_DEFAULTS.

2. get_slice_numpy: add a per-NDArray lock around the array-level read

b2nd_get_slice_cbuffer has no context parameter to make it thread-local,
and concurrent calls against the same NDArray segfault. Added a
per-instance PyThread_type_lock, acquired only around this call.
Independent NDArray objects still read fully in parallel; only concurrent
reads on the same object serialize.

Test plan

  • Existing regression tests + a concurrent stress script pass repeatedly
    (3x) on a free-threaded CPython 3.14t build under -X gil=0.
  • Full test suite passes on both a standard build and the free-threaded
    build under -X gil=0, with no new failures relative to unpatched
    main on the same interpreters.

Notes

  • This does not close Add support for the free-threaded build #555: the full suite still crashes elsewhere under
    -X gil=0 (a segfault in SChunk.__setitem__'s write path, unrelated to
    these two read-path fixes). I confirmed that crash reproduces on unpatched
    main as well, so it's a separate, pre-existing gap — out of scope here.
  • Disclosure: I used AI-assisted tooling to investigate and draft this fix;
    I've reviewed, tested, and can explain every change above.

get_1d_span_numpy borrowed the SChunk's shared dctx when non-NULL, but a
Blosc2 decompression context is mutable. Concurrent calls from multiple
threads (as done by the indexing query planner) can corrupt each other's
decoded output. Always create a private per-call context instead, still
associated with the SChunk via dparams.schunk so codecs/filters that
resolve per-schunk state during decompression keep working.
b2nd_get_slice_cbuffer has no per-call context parameter, so concurrent
reads against the same NDArray corrupt shared native state and segfault.
Add a per-instance lock around this call; independent NDArray objects are
unaffected and still read in parallel.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The lock must cover aliased NDArray wrappers, and private contexts must preserve configured SChunk decompression parameters.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity

Open (2)
What changed in this PR

This PR addresses two NDArray read-path thread-safety issues affecting free-threaded Python.

Changes:

  • Adds locking around slice reads.
  • Creates private decompression contexts for span reads.
  • Handles lock allocation and cleanup.
File Summary
src/​blosc2/​blosc2_ext.pyx Adds NDArray read synchronization and private decompression contexts.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/blosc2/blosc2_ext.pyx
Comment thread src/blosc2/blosc2_ext.pyx Outdated
@Johnny-Kao

Copy link
Copy Markdown
Contributor Author

Addressed both Copilot findings in 4a88750 and replied in the two review threads. Local validation passed: 123 adjacent NDArray/CTable tests on regular CPython and again with the GIL disabled on CPython 3.14 free-threaded, plus Ruff. The GitHub Actions workflows for this new fork commit are currently marked action_required (awaiting approval); could a maintainer approve them when convenient?

@FrancescAlted

Copy link
Copy Markdown
Member

Thanks @Johnny-Kao ! Your PR looks pretty well to me. My AI agent is suggesting this:

"""

  • Potential deadlock: the new lock acquisition (/private/tmp/python-blosc2-pr713/src/blosc2/blosc2_ext.pyx:3867) blocks while holding the GIL. If thread A holds that lock and its Python postfilter releases the GIL—for example during NumPy work—thread B can acquire the GIL and block on the lock. A then cannot resume. Acquire the lock inside with nogil:; the read itself can retain its current GIL handling.

  • Concurrency coverage is thin: the new test exercises shared views, but it neither forces overlapping reads nor covers concurrent get_1d_span_numpy calls. A targeted regression for the callback deadlock would materially improve confidence.

The good parts are preserving the SChunk’s decompression parameters, giving span reads private contexts, and sharing the lock across views while retaining the base’s lifetime.
"""

I’d also benchmark repeated small span reads: creating a decompression context per call is correct in principle, but could add noticeable indexing overhead.

@Johnny-Kao

Johnny-Kao commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review — I have addressed the GIL-deadlock concern by acquiring the shared NDArray read lock inside with nogil, and added a concurrent get_1d_span_numpy regression test (skipped on WASM, where Python threads cannot start).

I also considered removing the shared lock to improve parallel slice-read throughput. However, b2nd_get_slice_cbuffer still operates on shared SChunk/b2nd read state; making general slice reads safely parallel would require independent per-read/per-thread b2nd contexts and their lifecycle in the underlying API. That is a broader C-Blosc2/b2nd design change, so I have deliberately kept this PR focused on the correctness fix. The span path already uses a private decompression context and is now covered for concurrent calls.

@FrancescAlted
FrancescAlted merged commit bf8ae3c into Blosc:main Sep 24, 2026
36 checks passed
@FrancescAlted

Copy link
Copy Markdown
Member

Thank you @Johnny-Kao !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for the free-threaded build

3 participants