GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — ctypes: fix Windows pip truststore by youknowone · Pull Request #8314 · RustPython/RustPython Conversation
|
|
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⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds Changesctypes behavior and Windows validation
Native thread VM state preservation
Sequence Diagram(s)sequenceDiagram
participant NativeThread
participant PyEval_SaveThread
participant SavedThreadState
participant PyEval_RestoreThread
NativeThread->>PyEval_SaveThread: detach and save VM state
PyEval_SaveThread->>SavedThreadState: allocate saved state payload
PyEval_SaveThread-->>NativeThread: return thread-state pointer
NativeThread->>PyEval_RestoreThread: provide thread-state pointer
PyEval_RestoreThread->>SavedThreadState: reconstruct saved payload
PyEval_RestoreThread-->>NativeThread: restore and reattach VM state
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
9b3ae9d to
7eed565
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/capi/src/pystate.rs (1)
101-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a direct
PyEval_SaveThread/PyEval_RestoreThreadregression test.This test exercises
Python::attach/detachand stop-the-world transitions, but doesn't directly call the newly-implementedPyEval_SaveThread/PyEval_RestoreThreadpair, so the actual save/restore payload round-trip (and the gap noted inthread.rs) isn't covered.#[test] fn save_restore_thread() { Python::attach(|_py| { assert!(current_vm_is_set()); let saved = PyEval_SaveThread(); assert!(!current_vm_is_set()); unsafe { PyEval_RestoreThread(saved) }; assert!(current_vm_is_set()); }); }🤖 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/capi/src/pystate.rs` around lines 101 - 126, Add a direct regression test for the PyEval_SaveThread/PyEval_RestoreThread pair alongside the existing thread-state test. Within Python::attach, assert the VM is set, save the thread state, assert it is cleared, restore the saved payload, and assert the VM is set again; preserve the unsafe restore call as required by the API.
🤖 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/vm/thread.rs`:
- Around line 251-280: Update restore_current_thread and the existing VmRef::new
and attach_current_thread paths to share an attach_vm_to_current_thread helper
that initializes the thread slot with init_thread_slot_if_needed before calling
attach_thread. Ensure the helper accepts the VM to attach and preserves each
caller’s existing VM source, so restoring state on a different native thread
registers it for ATTACHED/QSBR tracking before updating the VM stack.
---
Nitpick comments:
In `@crates/capi/src/pystate.rs`:
- Around line 101-126: Add a direct regression test for the
PyEval_SaveThread/PyEval_RestoreThread pair alongside the existing thread-state
test. Within Python::attach, assert the VM is set, save the thread state, assert
it is cleared, restore the saved payload, and assert the VM is set again;
preserve the unsafe restore call as required by the API.
🪄 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: e979c9cc-fe81-4dc5-a393-410cb3d16348
📒 Files selected for processing (2)
crates/capi/src/pystate.rscrates/vm/src/vm/thread.rs
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| /// Restore a VM context previously returned by [`save_current_thread`]. | ||
| #[cfg(feature = "threading")] | ||
| pub fn restore_current_thread(state: SavedThreadState) { | ||
| assert!( | ||
| !current_vm_is_set(), | ||
| "restore_current_thread() called with an attached VM" | ||
| ); | ||
| let SavedThreadState { | ||
| vm_stack, | ||
| gilstate_vm, | ||
| } = state; | ||
| let vm = vm_stack | ||
| .last() | ||
| .copied() | ||
| .expect("saved thread state has no VM"); | ||
|
|
||
| GILSTATE_VM.with(|current| { | ||
| let mut current = current.borrow_mut(); | ||
| assert!( | ||
| current.is_none(), | ||
| "restore_current_thread() called with a GILState VM" | ||
| ); | ||
| *current = gilstate_vm; | ||
| }); | ||
|
|
||
| // SAFETY: borrowed VMs remain alive for the dynamic save/restore scope, | ||
| // while an owned GILState VM was restored above before this dereference. | ||
| attach_thread(unsafe { vm.as_ref() }); | ||
| VM_STACK.with(|vms| *vms.borrow_mut() = vm_stack); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
restore_current_thread skips thread-slot initialization before attach_thread.
attach_thread (Line 380-410) only performs the ATTACHED/DETACHED CAS and QSBR.online when CURRENT_THREAD_SLOT is already Some; if the slot is None it silently no-ops. Every other path that calls attach_thread — VmRef::new (Line 183-203) and attach_current_thread (context snippet) — first calls init_thread_slot_if_needed(vm). restore_current_thread calls attach_thread directly without that init step.
This is safe only when restore always runs on the exact same native thread that previously called save_current_thread (whose slot was already initialized before the save). But CPython's own PyEval_RestoreThread contract explicitly allows attaching "whichever thread calls it" — i.e. handing the saved state to a different native thread is a legitimate, documented use case. On such a thread, this gap leaves the thread un-registered for stop-the-world/QSBR tracking while VM_STACK/current_vm_is_set() reports it as attached — a silent, hard-to-diagnose state divergence rather than a clear panic.
As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code" — extracting a shared attach_vm_to_current_thread helper (used by VmRef::new, attach_current_thread, and restore_current_thread) would both fix this gap and remove the duplication.
🔧 Proposed fix
// SAFETY: borrowed VMs remain alive for the dynamic save/restore scope,
// while an owned GILState VM was restored above before this dereference.
- attach_thread(unsafe { vm.as_ref() });
+ let vm_ref = unsafe { vm.as_ref() };
+ init_thread_slot_if_needed(vm_ref);
+ attach_thread(vm_ref);
VM_STACK.with(|vms| *vms.borrow_mut() = vm_stack);📝 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.
| /// Restore a VM context previously returned by [`save_current_thread`]. | |
| #[cfg(feature = "threading")] | |
| pub fn restore_current_thread(state: SavedThreadState) { | |
| assert!( | |
| !current_vm_is_set(), | |
| "restore_current_thread() called with an attached VM" | |
| ); | |
| let SavedThreadState { | |
| vm_stack, | |
| gilstate_vm, | |
| } = state; | |
| let vm = vm_stack | |
| .last() | |
| .copied() | |
| .expect("saved thread state has no VM"); | |
| GILSTATE_VM.with(|current| { | |
| let mut current = current.borrow_mut(); | |
| assert!( | |
| current.is_none(), | |
| "restore_current_thread() called with a GILState VM" | |
| ); | |
| *current = gilstate_vm; | |
| }); | |
| // SAFETY: borrowed VMs remain alive for the dynamic save/restore scope, | |
| // while an owned GILState VM was restored above before this dereference. | |
| attach_thread(unsafe { vm.as_ref() }); | |
| VM_STACK.with(|vms| *vms.borrow_mut() = vm_stack); | |
| } | |
| /// Restore a VM context previously returned by [`save_current_thread`]. | |
| #[cfg(feature = "threading")] | |
| pub fn restore_current_thread(state: SavedThreadState) { | |
| assert!( | |
| !current_vm_is_set(), | |
| "restore_current_thread() called with an attached VM" | |
| ); | |
| let SavedThreadState { | |
| vm_stack, | |
| gilstate_vm, | |
| } = state; | |
| let vm = vm_stack | |
| .last() | |
| .copied() | |
| .expect("saved thread state has no VM"); | |
| GILSTATE_VM.with(|current| { | |
| let mut current = current.borrow_mut(); | |
| assert!( | |
| current.is_none(), | |
| "restore_current_thread() called with a GILState VM" | |
| ); | |
| *current = gilstate_vm; | |
| }); | |
| // SAFETY: borrowed VMs remain alive for the dynamic save/restore scope, | |
| // while an owned GILState VM was restored above before this dereference. | |
| let vm_ref = unsafe { vm.as_ref() }; | |
| init_thread_slot_if_needed(vm_ref); | |
| attach_thread(vm_ref); | |
| VM_STACK.with(|vms| *vms.borrow_mut() = vm_stack); | |
| } |
🤖 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/thread.rs` around lines 251 - 280, Update
restore_current_thread and the existing VmRef::new and attach_current_thread
paths to share an attach_vm_to_current_thread helper that initializes the thread
slot with init_thread_slot_if_needed before calling attach_thread. Ensure the
helper accepts the VM to attach and preserves each caller’s existing VM source,
so restoring state on a different native thread registers it for ATTACHED/QSBR
tracking before updating the VM stack.
Source: Coding guidelines
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
|
@bschoenmaeckers Could you please review if C API changes are reasonable? |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
|
|
||
| #[repr(C)] | ||
| struct SavedPyThreadState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be merged into the PyThreadState struct. You may add private fields after the interp pub field.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much!
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
c7d7ece to
c200885
Compare
Uh oh!
There was an error while loading. Please reload this page.
* ctypes: preserve result when errcheck returns args * ctypes: cover Windows pip truststore * capi: implement PyEval_SaveThread * capi: merge saved thread state into PyThreadState
Reviewers
+1 more reviewer
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
pip is failing to run on Windows 11 25H2.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.