Repository navigation
Fix thread UnraisableException object to be None - #8326
Conversation
📝 WalkthroughWalkthroughThread exception reporting is centralized through a shared helper for plain and joinable threads. VM stderr formatting now handles unraisable reports with no associated object while preserving representation output for object-backed reports. ChangesThread exception reporting
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ThreadRunner
participant report_unraisable_thread_exception
participant VirtualMachine
ThreadRunner->>report_unraisable_thread_exception: uncaught non-SystemExit exception
report_unraisable_thread_exception->>VirtualMachine: run_unraisable with thread message
VirtualMachine->>VirtualMachine: write_unraisable_to_stderr
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_thread.py (TODO: 3) dependencies: dependent tests: (14 tests) Legend:
|
Assisted-by: Codex:gpt-5
67164b6 to
dbaa407
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/stdlib/_thread.rs`:
- Around line 569-574: Update the message construction in the thread exception
path around func.as_ref().repr(vm) so repr failures produce a fallback context
string rather than None. Preserve the existing formatted representation when
repr succeeds, and always pass a context message to vm.run_unraisable.
In `@crates/vm/src/vm/mod.rs`:
- Around line 1299-1303: Update the None-object branch in the default unraisable
hook to write msg followed only by a newline, removing the trailing colon from
the format passed to write_to_stderr. Preserve the existing conditional behavior
and formatting for cases where an object is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0819d4a1-51ee-4fa0-9993-90f546cfd998
⛔ Files ignored due to path filters (1)
Lib/test/test_thread.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/vm/src/stdlib/_thread.rscrates/vm/src/vm/mod.rs
| let msg = func | ||
| .as_ref() | ||
| .repr(vm) | ||
| .ok() | ||
| .map(|repr| format!("Exception ignored in thread started by {}", repr.as_wtf8())); | ||
| vm.run_unraisable(exc, msg, vm.ctx.none()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle repr failures gracefully to ensure the context message is printed.
If func.as_ref().repr(vm) fails, ok() maps the result to None, which suppresses the "Exception ignored in thread started by..." context message entirely. CPython prevents this by using a fallback message when the function's representation cannot be retrieved.
🐛 Proposed fix to provide a fallback
- let msg = func
- .as_ref()
- .repr(vm)
- .ok()
- .map(|repr| format!("Exception ignored in thread started by {}", repr.as_wtf8()));
- vm.run_unraisable(exc, msg, vm.ctx.none());
+ let msg = match func.as_ref().repr(vm) {
+ Ok(repr) => format!("Exception ignored in thread started by {}", repr.as_wtf8()),
+ Err(_) => "Exception ignored in thread started by <function repr() failed>".to_owned(),
+ };
+ vm.run_unraisable(exc, Some(msg), vm.ctx.none());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let msg = func | |
| .as_ref() | |
| .repr(vm) | |
| .ok() | |
| .map(|repr| format!("Exception ignored in thread started by {}", repr.as_wtf8())); | |
| vm.run_unraisable(exc, msg, vm.ctx.none()); | |
| let msg = match func.as_ref().repr(vm) { | |
| Ok(repr) => format!("Exception ignored in thread started by {}", repr.as_wtf8()), | |
| Err(_) => "Exception ignored in thread started by <function repr() failed>".to_owned(), | |
| }; | |
| vm.run_unraisable(exc, Some(msg), vm.ctx.none()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/stdlib/_thread.rs` around lines 569 - 574, Update the message
construction in the thread exception path around func.as_ref().repr(vm) so repr
failures produce a fallback context string rather than None. Preserve the
existing formatted representation when repr succeeds, and always pass a context
message to vm.run_unraisable.
| if self.is_none(object) { | ||
| if let Some(msg) = msg { | ||
| write_to_stderr(&format!("{msg}:\n"), &stderr, self); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the trailing colon when formatting msg without an object.
When object is None, the default unraisable hook should write the message followed by a newline, without a trailing colon. Currently, a thread exception will be incorrectly logged as Exception ignored in thread started by <function>: instead of matching CPython's format.
🐛 Proposed fix
if self.is_none(object) {
if let Some(msg) = msg {
- write_to_stderr(&format!("{msg}:\n"), &stderr, self);
+ write_to_stderr(&format!("{msg}\n"), &stderr, self);
}
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.is_none(object) { | |
| if let Some(msg) = msg { | |
| write_to_stderr(&format!("{msg}:\n"), &stderr, self); | |
| } | |
| } else { | |
| if self.is_none(object) { | |
| if let Some(msg) = msg { | |
| write_to_stderr(&format!("{msg}\n"), &stderr, self); | |
| } | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/vm/mod.rs` around lines 1299 - 1303, Update the None-object
branch in the default unraisable hook to write msg followed only by a newline,
removing the trailing colon from the format passed to write_to_stderr. Preserve
the existing conditional behavior and formatting for cases where an object is
present.
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
huh, didn't know about this. Thank you!
Summary
_threadcallbacksNoneas the unraisable exception'sobjectreport_unraisable_thread_exception()Summary by CodeRabbit
stderrformatting when no associated object is available: messages are now shorter and avoid unnecessary prefixes when appropriate.