Fix thread teardown panic when weakref callback fires during cleanup by JamesClarke7283 · Pull Request #7965 · RustPython/RustPython

JamesClarke7283 · 2026-05-24T08:28:31Z

Summary

threading.Thread(target=fn).start() panics in debug builds during thread teardown:

thread '<unnamed>' panicked at crates/vm/src/vm/thread.rs:451:13:
push_thread_frame called without initialized thread slot

Root cause

In both _thread spawn paths (start_new_thread → run_thread, and start_joinable_thread's spawn closure), the user func: ArgCallable (holding a PyObjectRef) drops after cleanup_current_thread_frames(vm) has set CURRENT_THREAD_SLOT = None:

In run_thread, func is a function parameter — it drops at end-of-function, after the inline cleanup.
In the start_joinable_thread spawn closure, func is a move-capture — and closure captures drop after all locals at end-of-body, so they drop after the scopeguard::defer! guard fires the cleanup.

When func finally drops, the inner PyBoundMethod is freed, triggering WeakRefList::clear → with_vm → a Python weakref callback → push_thread_frame, which finds the slot already None and hits the debug_assert!.

Fix

Move each func.invoke(...) match into an inner block with let func = func;. That moves func into a local declared inside the inner scope, so it drops at the end of that scope — before the cleanup tears down CURRENT_THREAD_SLOT.

Two small reorderings in crates/vm/src/stdlib/_thread.rs. No API or lifecycle changes; the debug_assert! in push_thread_frame/pop_thread_frame stays as a safety net.

Test plan

Issue reproducer panics before fix, passes after fix (verified across 5 runs).
Stress test (50 threads, mixed join/no-join): passes.
Stress test (20 no-join threads in a loop): passes.
Explicit weakref-callback-during-teardown test: passes.
extra_tests/snippets/test_threading.py: exit 0.
python -m test test_thread (24 tests): all pass.
python -m test test_threading (223 tests): all pass.
python -m test test_threading_local test_threadedtempfile: all pass.
cargo check -p rustpython-vm -p rustpython-stdlib: clean.

Note: extra_tests/snippets/stdlib_threading.py segfaults on both main and this branch identically (an unrelated pre-existing issue in the fork/multiprocessing path) — not introduced by this change.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved thread teardown so thread targets are dropped before cleanup, preventing teardown-time interference and ensuring reliable shutdown.
Tests
- Added regression tests for thread join ordering and for thread exit behavior when not explicitly joined; removed an older redundant threading test.
Chores
- Updated ignore list to exclude a scheduled-tasks lock file.

- Drop the target function inside an inner scope so its Python refs are released before cleanup_current_thread_frames clears CURRENT_THREAD_SLOT - Avoids panic in push_thread_frame when a weakref callback runs as `func` drops at end-of-function

coderabbitai · 2026-05-24T08:28:53Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: d4b82274-8abf-4add-8554-802f37aba164

📥 Commits

Reviewing files that changed from the base of the PR and between de59498 and 5e07d34.

📒 Files selected for processing (1)

.gitignore

✅ Files skipped from review due to trivial changes (1)

.gitignore

📝 Walkthrough

Walkthrough

Thread-target invocations are run inside inner scopes so the Python callable drops before thread-slot/frame-tracking teardown; two regression tests verify join ordering and clean teardown for unjoined threads.

Changes

Thread teardown safety

Layer / File(s)	Summary
Scoped function execution to prevent weakref panics `crates/vm/src/stdlib/_thread.rs`	`run_thread` and the joinable-thread deferred cleanup wrap `func.invoke(...)` in inner scopes so the Python callable is dropped before sentinel unlocking, `cleanup_thread_local_data()`, `cleanup_current_thread_frames(vm)`, and decrementing `thread_count`.
Threading regression tests and .gitignore `extra_tests/snippets/stdlib_threading.py`, `.gitignore`	Added `import time`, two tests (`thread_join_ordering`, `thread_exit_without_join`) that validate ordering and clean teardown when a thread is not joined; updated `.gitignore` to ignore `.claude/scheduled_tasks.lock`.

Sequence Diagram(s)

sequenceDiagram
  participant Starter as ThreadStarter (spawn/join)
  participant Runner as run_thread / start_joinable_thread
  participant Func as func.invoke(...)
  participant Teardown as ThreadTeardown
  Starter->>Runner: spawn thread
  Runner->>Func: call func.invoke(...) inside inner scope
  Func-->>Teardown: func object drops (on inner-scope exit)
  Teardown->>Teardown: unlock sentinel / cleanup_thread_local_data / cleanup_current_thread_frames / decrement thread_count
  Starter->>Starter: join or wait for completion

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰
I hopped into threads with care and hops,
Scoped the function so no teardown flops.
Drop comes first, then cleanup sings,
No weakref panic, only calm things.
🥕👏

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Out of Scope Changes check	❓ Inconclusive	The PR includes minimal scoped changes: thread teardown fixes in _thread.rs, regression tests in stdlib_threading.py, cleanup of old test in test_threading.py, and an unrelated .gitignore entry.	The .gitignore change for .claude/scheduled_tasks.lock appears unrelated to the threading fix; clarify if this is intentional or should be removed in a separate commit.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly reflects the main fix: preventing panics during thread teardown when weakref callbacks fire, which is the core issue addressed.
Linked Issues check	✅ Passed	The PR addresses all objectives from issue `#7813`: reproduces and fixes the panic, ensures CPython-like behavior, prevents VM frame operations with uninitialized slots, and makes object cleanup safe during teardown with regression tests.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

ShaharNaveh

Please add a test case to extra_tests/ so we won't have regression in the future:)

JamesClarke7283 · 2026-05-24T08:34:30Z

Please add a test case to extra_tests/ so we won't have regression in the future:)

Will do

@ShaharNaveh

- Rewrite both match blocks as `if let Err(exc) && !is(SystemExit)` per @ShaharNaveh - Add extra_tests/snippets/test_threading_teardown.py regression test from issue RustPython#7813 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

{"subject": "Use double quotes in threading teardown test", "body": ""}

Merge the contents of test_threading.py and test_threading_teardown.py into stdlib_threading.py to match the file's helper-function style; the two source snippets are deleted. The bodies become `thread_join_ordering` (start/join ordering) and `thread_exit_without_join` (regression test for RustPython#7813 — no-join thread teardown must not panic). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

ShaharNaveh reviewed May 24, 2026

View reviewed changes

Comment thread crates/vm/src/stdlib/_thread.rs Outdated

ShaharNaveh reviewed May 24, 2026

View reviewed changes

Comment thread crates/vm/src/stdlib/_thread.rs Outdated

ShaharNaveh requested changes May 24, 2026

View reviewed changes

JamesClarke7283 and others added 4 commits May 24, 2026 09:42

Commit message

f862ed2

{"subject": "Use double quotes in threading teardown test", "body": ""}

Add scheduled tasks lock file

29ae378

ShaharNaveh requested changes May 24, 2026

View reviewed changes

Comment thread .claude/scheduled_tasks.lock Outdated

Added .claude/scheduled_tasks.lock to .gitignore file

5e07d34

ShaharNaveh approved these changes May 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix thread teardown panic when weakref callback fires during cleanup#7965

Fix thread teardown panic when weakref callback fires during cleanup#7965
JamesClarke7283 wants to merge 6 commits into
RustPython:mainfrom
JamesClarke7283:fix-thread-panic

JamesClarke7283 commented May 24, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 24, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

Uh oh!

Uh oh!

ShaharNaveh left a comment

Uh oh!

JamesClarke7283 commented May 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

JamesClarke7283 commented May 24, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root cause

Fix

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

Uh oh!

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

JamesClarke7283 commented May 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JamesClarke7283 commented May 24, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 24, 2026 •

edited

Loading