Skip to content

fix: reset the codec-pipeline thread pool after fork - #4483

Merged
d-v-b merged 3 commits into
zarr-developers:mainfrom
barlowa124:fork-codec-pool-4478
Oct 9, 2026
Merged

d-v-b merged 3 commits into
zarr-developers:mainfrom
barlowa124:fork-codec-pool-4478

Conversation

@barlowa124

@barlowa124 barlowa124 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #4478. zarr.core.codec_pipeline keeps a module-level ThreadPoolExecutor for FusedCodecPipeline. A forked child inherits _pool pointing at threads that do not exist in the child, plus the parent's idle-semaphore permits. submit consumes a permit and returns without spawning a thread, so submissions up to the permit count queue work that nothing runs. Small multi-chunk reads hang forever. Larger reads recover once they exhaust the stale permits and spawn a real thread.

reset_resources_after_fork in zarr.core.sync resets the event loop and IO thread plus the sync executor after a fork, but the codec pipeline registered no after_in_child hook. This adds one that drops the inherited _pool and _pool_size and recreates _pool_lock, since the parent may have held it at fork time, which would leave the child's copy permanently locked.

The regression test reads a multi-chunk array in the parent to populate the pool, then reads a 2-chunk region in a forked child. Two chunks is below the inherited permit count, so unfixed code hangs and the test fails on a 30s get timeout instead of blocking CI.

For reviewers

Two things I would value a second look at are the decision to recreate _pool_lock rather than reuse the inherited lock, and whether any other module-level executor state needs the same treatment. Locally the issue's ProcessPoolExecutor script timed out at 30s unfixed and passed with the fix. The permit arithmetic was confirmed by printing _idle_semaphore._value in parent and child.

Author attestation

  • I am a human, these are my changes, and I have reviewed and understood every change and can explain why each is correct.

TODO

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/user-guide/*.md
  • Changes documented as a new file in changes/
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

barlowa124 and others added 2 commits October 6, 2026 18:11
A forked child inherits codec_pipeline._pool pointing at threads that
do not exist in the child, plus the parent's idle-semaphore permits.
Submissions up to the permit count are consumed without spawning a
thread, so small FusedCodecPipeline reads hang forever.

Add an after_in_child hook that drops the inherited pool, size, and
lock. The lock is recreated because it may have been held by a thread
that no longer exists in the child.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@github-actions github-actions Bot added needs release notes Automatically applied to PRs which haven't added release notes and removed needs release notes Automatically applied to PRs which haven't added release notes labels Oct 6, 2026
@codecov

codecov Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.71%. Comparing base (ab4f2fe) to head (8d225da).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4483   +/-   ##
=======================================
  Coverage   94.71%   94.71%           
=======================================
  Files          94       94           
  Lines       13659    13663    +4     
=======================================
+ Hits        12937    12941    +4     
  Misses        722      722           
Files with missing lines Coverage Δ
src/zarr/core/codec_pipeline.py 95.58% <100.00%> (+0.03%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@d-v-b d-v-b 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.

looks good, thank you!

@d-v-b
d-v-b enabled auto-merge (squash) October 9, 2026 17:21
@d-v-b
d-v-b merged commit 6599477 into zarr-developers:main Oct 9, 2026
39 checks passed
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.

FusedCodecPipeline: forked worker hangs on a multi-chunk read; reset_resources_after_fork misses codec_pipeline._pool

2 participants