_thread: initialize local subclasses per thread - #8353
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesThread-local initialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Local
participant LocalState
participant CustomInit
Caller->>Local: construct with init_args
Local->>LocalState: store init_args
Local->>LocalState: create current-thread dict
Local->>CustomInit: invoke custom __init__
CustomInit-->>Local: return success or error
Local->>LocalState: remove dict on initialization error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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] lib: cpython/Lib/sqlite3 dependencies:
dependent tests: (2 tests)
[x] lib: cpython/Lib/io.py dependencies:
dependent tests: (108 tests)
[x] lib: cpython/Lib/struct.py dependencies:
dependent tests: (179 tests)
[x] lib: cpython/Lib/email dependencies: dependent tests: (53 tests)
[x] lib: cpython/Lib/threading.py dependencies:
dependent tests: (163 tests)
[x] lib: cpython/Lib/ast.py dependencies:
dependent tests: (149 tests)
[x] lib: cpython/Lib/shlex.py dependencies:
dependent tests: (10 tests)
Legend:
|
3fda9b0 to
0ba28a7
Compare
|
Hello, @ShaharNaveh 😄 I opted for If you don't mind, I'd love to get your thoughts on this: Is adding |
| unsafe impl Traverse for LocalState { | ||
| fn traverse(&self, tracer_fn: &mut TraverseFn<'_>) { | ||
| self.init_args.traverse(tracer_fn); | ||
| for (_, dict) in &self.dicts { |
There was a problem hiding this comment.
This may fix the two Clippy lints, but it may not work:
| for (_, dict) in &self.dicts { | |
| let mut dicts: Vec<_> = &self.dicts.values().collect(); | |
| dicts.sort(); | |
| for dict in dicts { |
There was a problem hiding this comment.
I did consider using .collect(), but I was worried about the cost of allocating a new Vec here. 🥲
There was a problem hiding this comment.
that's right, let's avoid unnecessary allocation
0ba28a7 to
2da62c8
Compare
![coderabbitai[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/stdlib/_thread.rs (1)
928-937: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid dropping guards while
LOCAL_GUARDSis borrowed.
cleanup_thread_local_data()clearsLOCAL_GUARDS.borrow_mut(), and eachLocalGuard::drop()removes a Python dict; the dict’s__del__()can synchronously access a different_local, creating the same guard and calling the borrowedLOCAL_GUARDSRefCellagain (Already borrowed: BorrowMutError). Clone/clear just the data here or take the guard out first, likeremove_current_dict()does.🤖 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 928 - 937, The LocalGuard::drop cleanup can trigger __del__ while LOCAL_GUARDS is mutably borrowed, causing reentrant BorrowMutError. Update the LocalGuard drop path used by cleanup_thread_local_data() to remove or take the thread-local guard/data before dropping the Python dict, following the safe ownership pattern in remove_current_dict() so destructor execution occurs after the LOCAL_GUARDS borrow ends.
🧹 Nitpick comments (1)
crates/vm/src/stdlib/_thread.rs (1)
939-958: 🚀 Performance & Scalability | 🔵 TrivialRe: HashMap vs BTreeMap/IndexMap for
LocalState.dicts.Since
traverse/clearonly need completeness (not ordering) for GC correctness, anddictsis otherwise used purely for keyed lookup (get/entry/remove),HashMapwith the O(1) average lookup is the right choice here —BTreeMap/IndexMapwould only add overhead (O(log n) lookups, or an extra dependency) without a corresponding benefit. KeepingHashMap+#[allow(clippy::iter_over_hash_type)]is reasonable.One small nit: the codebase already has precedent for the "unordered hash iteration" allowance using
#[expect(clippy::iter_over_hash_type, reason = "...")](e.g.crates/vm/src/builtins/frame.rs), which documents why order doesn't matter. Matching that style here would be slightly more consistent/self-documenting than a bare#[allow(...)].🤖 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 939 - 958, Replace the bare #[allow(clippy::iter_over_hash_type)] on LocalState::traverse with the codebase’s #[expect(..., reason = "...")] style, documenting that dict traversal order is irrelevant because GC only requires complete visitation. Keep the HashMap implementation and traversal logic unchanged.
🤖 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.
Outside diff comments:
In `@crates/vm/src/stdlib/_thread.rs`:
- Around line 928-937: The LocalGuard::drop cleanup can trigger __del__ while
LOCAL_GUARDS is mutably borrowed, causing reentrant BorrowMutError. Update the
LocalGuard drop path used by cleanup_thread_local_data() to remove or take the
thread-local guard/data before dropping the Python dict, following the safe
ownership pattern in remove_current_dict() so destructor execution occurs after
the LOCAL_GUARDS borrow ends.
---
Nitpick comments:
In `@crates/vm/src/stdlib/_thread.rs`:
- Around line 939-958: Replace the bare #[allow(clippy::iter_over_hash_type)] on
LocalState::traverse with the codebase’s #[expect(..., reason = "...")] style,
documenting that dict traversal order is irrelevant because GC only requires
complete visitation. Keep the HashMap implementation and traversal logic
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: ddfe89ed-cc11-4bcc-9573-874d048e779d
⛔ Files ignored due to path filters (1)
Lib/test/test_threading_local.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/vm/src/stdlib/_thread.rs
Assisted-by: Codex:gpt-5.6
da901dd to
7653689
Compare
Assisted-by: Codex:gpt-5.6
4 participants
Assisted-by: Codex:gpt-5.6
Summary
_thread._localwhen no custom__init__is provided__init__when a thread first accesses the local objectThreadLocalTestargument and cycle-collection testsSummary by CodeRabbit
_thread.localso constructor arguments are correctly applied to each thread’s instance state._thread.localconstruction from leaving partial per-thread state, and improved garbage-collection/cleanup behavior to avoid stale data.