thread.setname and context - #6726
Conversation
Sorry, something went wrong.
|
Caution Review failedThe pull request is closed. WalkthroughExtends 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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review details Configuration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 890b018 and d22c48b. 📒 Files selected for processing (1)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings. 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)crates/vm/src/stdlib/thread.rs (1)📜 Review details319-327: Consider adding truncation for macOS thread names.
macOS pthread_setname_np has 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.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 2e5257d and 890b018.
📒 Files selected for processing (2)- crates/stdlib/src/contextvars.rs
- crates/vm/src/stdlib/thread.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by running cargo fmt to 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.rs
- crates/stdlib/src/contextvars.rs
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
crates/stdlib/src/mmap.rs (2)🔇 Additional comments (8)crates/stdlib/src/select.rs (2)
- __enter__ (1257-1260)
- __exit__ (1263-1265)
- __enter__ (723-726)
- __exit__ (729-736)
crates/vm/src/stdlib/thread.rs (3)crates/stdlib/src/contextvars.rs (5)12-12: LGTM!
The import addition of PyStrRef is needed to support the new set_name function parameter type.
171-174: LGTM!
The locked method correctly exposes the lock state and follows the existing pattern used in repr_lock_impl! and release methods.
263-266: LGTM!
The locked method implementation for RLock is consistent with the Lock implementation.
27-32: LGTM!
Import additions for PyList and Iterable are necessary to support the new Iterable implementation for PyContext.
166-166: LGTM!
Adding Iterable to the pyclass decorator properly integrates the new iteration capability.
237-244: LGTM!
The items method implementation is consistent with the existing keys and values methods. The TODO comment appropriately notes the return type should eventually be a proper view type.
288-295: LGTM!
The Iterable implementation correctly creates a snapshot of keys and delegates to PyList's iterator. This approach is safe and avoids holding the borrow during iteration.
590-605: LGTM!
The context manager implementation for ContextToken is correct:
- __enter__ returns self following standard patterns (similar to mmap.rs and select.rs).
- __exit__ properly delegates to ContextVar::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.
Sorry, something went wrong.
| #[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 issue | 🟡 Minor
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
};‼️ 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()); | |
| } | |
| } | |
| } |
Sorry, something went wrong.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.