thread.setname and context #6726
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughExtends PyContext to support Python iteration and items(); adds ContextToken enter/exit for with-statement semantics; adds locked() on Lock/RLock and a platform-aware set_name(name: PyStrRef) thread API. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✏️ Tip: You can disable this entire section by setting 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 |
e325efd to
890b018
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/vm/src/stdlib/thread.rs (1)
319-327: Consider adding truncation for macOS thread names.macOS
pthread_setname_nphas a 64-byte limit. While less restrictive than Linux, names exceeding this limit will cause the call to fail silently. Consider adding truncation similar to the Linux implementation for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/stdlib/src/contextvars.rscrates/vm/src/stdlib/thread.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/stdlib/thread.rscrates/stdlib/src/contextvars.rs
🧠 Learnings (1)
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/stdlib/src/contextvars.rs
🧬 Code graph analysis (1)
crates/stdlib/src/contextvars.rs (2)
crates/stdlib/src/mmap.rs (2)
__enter__(1257-1260)__exit__(1263-1265)crates/stdlib/src/select.rs (2)
__enter__(723-726)__exit__(729-736)
🔇 Additional comments (8)
crates/vm/src/stdlib/thread.rs (3)
12-12: LGTM!The import addition of
PyStrRefis needed to support the newset_namefunction parameter type.
171-174: LGTM!The
lockedmethod correctly exposes the lock state and follows the existing pattern used inrepr_lock_impl!andreleasemethods.
263-266: LGTM!The
lockedmethod implementation forRLockis consistent with theLockimplementation.crates/stdlib/src/contextvars.rs (5)
27-32: LGTM!Import additions for
PyListandIterableare necessary to support the newIterableimplementation forPyContext.
166-166: LGTM!Adding
Iterableto thepyclassdecorator properly integrates the new iteration capability.
237-244: LGTM!The
itemsmethod implementation is consistent with the existingkeysandvaluesmethods. The TODO comment appropriately notes the return type should eventually be a proper view type.
288-295: LGTM!The
Iterableimplementation correctly creates a snapshot of keys and delegates toPyList's iterator. This approach is safe and avoids holding the borrow during iteration.
590-605: LGTM!The context manager implementation for
ContextTokenis correct:
__enter__returns self following standard patterns (similar tommap.rsandselect.rs).__exit__properly delegates toContextVar::reset, which handles restoring the previous value and marking the token as used to prevent reuse.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| #[cfg(target_os = "linux")] | ||
| { | ||
| use std::ffi::CString; | ||
| if let Ok(c_name) = CString::new(name.as_str()) { | ||
| // pthread_setname_np on Linux has a 16-byte limit including null terminator | ||
| let truncated = if c_name.as_bytes().len() > 15 { | ||
| CString::new(&c_name.as_bytes()[..15]).unwrap_or(c_name) | ||
| } else { | ||
| c_name | ||
| }; | ||
| unsafe { | ||
| libc::pthread_setname_np(libc::pthread_self(), truncated.as_ptr()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential UTF-8 boundary issue when truncating thread name on Linux.
The truncation at byte 15 may split a multi-byte UTF-8 character, producing an invalid UTF-8 sequence. While CString::new won't fail (it only checks for interior nulls), passing invalid UTF-8 to the OS could cause unexpected behavior.
Consider truncating at a valid UTF-8 character boundary:
Proposed fix
#[cfg(target_os = "linux")]
{
use std::ffi::CString;
if let Ok(c_name) = CString::new(name.as_str()) {
// pthread_setname_np on Linux has a 16-byte limit including null terminator
- let truncated = if c_name.as_bytes().len() > 15 {
- CString::new(&c_name.as_bytes()[..15]).unwrap_or(c_name)
+ let truncated = if name.as_str().len() > 15 {
+ // Find a valid UTF-8 boundary at or before byte 15
+ let s = name.as_str();
+ let mut end = 15;
+ while end > 0 && !s.is_char_boundary(end) {
+ end -= 1;
+ }
+ CString::new(&s[..end]).unwrap_or(c_name)
} else {
c_name
};📝 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.
| #[cfg(target_os = "linux")] | |
| { | |
| use std::ffi::CString; | |
| if let Ok(c_name) = CString::new(name.as_str()) { | |
| // pthread_setname_np on Linux has a 16-byte limit including null terminator | |
| let truncated = if c_name.as_bytes().len() > 15 { | |
| CString::new(&c_name.as_bytes()[..15]).unwrap_or(c_name) | |
| } else { | |
| c_name | |
| }; | |
| unsafe { | |
| libc::pthread_setname_np(libc::pthread_self(), truncated.as_ptr()); | |
| } | |
| } | |
| #[cfg(target_os = "linux")] | |
| { | |
| use std::ffi::CString; | |
| if let Ok(c_name) = CString::new(name.as_str()) { | |
| // pthread_setname_np on Linux has a 16-byte limit including null terminator | |
| let truncated = if name.as_str().len() > 15 { | |
| // Find a valid UTF-8 boundary at or before byte 15 | |
| let s = name.as_str(); | |
| let mut end = 15; | |
| while end > 0 && !s.is_char_boundary(end) { | |
| end -= 1; | |
| } | |
| CString::new(&s[..end]).unwrap_or(c_name) | |
| } else { | |
| c_name | |
| }; | |
| unsafe { | |
| libc::pthread_setname_np(libc::pthread_self(), truncated.as_ptr()); | |
| } | |
| } | |
| } |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.