Repository navigation
Fix stack overflow segfault on debug builds for Python threads - #7986
JamesClarke7283 wants to merge 7 commits into
Conversation
Rust's std::thread::Builder defaults to a 2 MB stack when no size is
set, which is too small for the call chains the Python stdlib runs on
helper threads in debug builds (e.g. test.test_ssl's threaded server).
CPython on glibc Linux relies on pthread's ~8 MB default instead.
Apply 8 MB as the default in apply_thread_stack_size when the user has
not explicitly called threading.stack_size(N). This matches CPython's
effective default across builds while preserving the existing API:
threading.stack_size() still returns 0 by default ("platform default"),
and explicit user values still take precedence.
Fixes RustPython#7941
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. ℹ️ Recent review info
Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely describes the main change: preventing stack-overflow segmentation faults for Python threads in debug builds. |
| Linked Issues check | ✅ Passed | Issue #7941 requires Python threaded tests to avoid debug-build stack overflows and segmentation faults. The PR applies an 8 MiB stack when no explicit stack size is configured, while preserving expli… |
| Out of Scope Changes check | ✅ Passed | The changes are limited to crates/vm/src/stdlib/_thread.rs. They implement the debug-build thread-stack fix for #7941 and add its regression test. The changes do not add unrelated behavior. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files. |
Finishing Touches
-
Resolve merge conflicts
-
- Resolve merge conflict in branch
fix-7941-thread-stack-overflow
- Resolve merge conflict in branch
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Satisfy docstring coverage check on the PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit applied an 8 MB default unconditionally, which slowed down multiprocessing tests on release: many forked children each spawning many threads with oversized virtual stack mappings caused the flaky MP CI step to time out (60 min, vs ~9 min on main). Issue RustPython#7941 only manifested in debug builds — release builds were already fine on Rust's 2 MB std default. Restrict the 8 MB override to `#[cfg(debug_assertions)]` so release behavior is unchanged from before the fix, while the original SSL segfault on debug stays fixed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
please also add failing test before patch |
This comment was marked as outdated.
This comment was marked as outdated.
Which test? the original one from the original issue that now passes or something different. |
Brings the branch up to date with main so CI job names match the required status checks (the windows snippets/cpython job was renamed by RustPython#8004 when its matrix skips were cleared). Also picks up RustPython#8018's _thread.rs stack-margin updates, which are orthogonal to the apply_thread_stack_size fix in this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
please add a test, which failed before the patch but success after the patch. If adding patch is not possible, please share discussion why it is impossible |
Verifies that a Python thread started without an explicit threading.stack_size() gets DEFAULT_THREAD_STACK_SIZE (8 MiB) in debug builds instead of Rust's 2 MiB std default, which overflowed and crashed in the SSL test server (RustPython#7941). The test reads back the spawned thread's actual stack size via pthread, fails before the fix and passes after it.
Done |
54f5615 to
a5e5bc6
Compare
Keep main's integrated debug stack fallback and regression coverage, including the minimum stack size for explicit requests. This resolves the conflicting older implementation without undoing upstream improvements. AI assistance: Codex resolved the merge and reviewed the stack-size implementation. Assisted-by: Codex:GPT-6
Summary
std::thread::Builderdefault stack of 2 MB, which is too small for the call chains the Python stdlib runs on helper threads in debug builds (where Rust stack frames are substantially larger). CPython on glibc Linux relies on pthread's ~8 MB default instead — a 4× difference that explains why this hits us but not CPython.apply_thread_stack_sizewhenever the user hasn't explicitly set a value viathreading.stack_size(N). This matches CPython's effective default while keeping the Python API contract exact:threading.stack_size()still returns0("platform default"), and explicit overrides still take precedence.crates/vm/src/stdlib/_thread.rs.Closes #7941
Test plan
cargo buildsucceeds.cargo run -- -m test.test_ssl ThreadedTests.test_socketserver→Ran 1 test in 21.59s, OK(was segfaulting onmain).threading.stack_size()returns0by default; setting to 1 MiB then reading back returns1048576; previous value reported correctly.🤖 Generated with Claude Code
Summary by CodeRabbit