Add minimal capi lifecycle support#7648
Add minimal capi lifecycle support#7648bschoenmaeckers wants to merge 5 commits intoRustPython:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant C as C client
participant T as Current Thread
participant M as MAIN_INTERP (OnceLock<Interpreter>)
C->>T: Py_Initialize() / Py_InitializeEx()
T->>M: initialize OnceLock (Interpreter::with_init(...))
alt interpreter initialized now
M->>M: construct Interpreter inside OnceLock
end
T->>M: request_vm_from_interpreter()
M->>M: lock Interpreter, enter(|vm| vm.new_thread())
M-->>T: return ThreadedVirtualMachine
T->>T: attach_vm_to_thread() (store thread-local VM)
C->>T: PyGILState_Ensure / refcount calls use thread-local VM
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
0a01cb4 to
2b4b20f
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/capi/src/pylifecycle.rs`:
- Around line 31-35: The current Py_InitializeEx function panics when
INITIALIZED.is_completed() is true, but per the CPython C API it must be a no-op
on repeated calls; modify Py_InitializeEx (and any Py_Initialize wrapper) to
return immediately when INITIALIZED.is_completed() is true instead of panicking
so repeated FFI calls do not abort the process, preserving existing
initialization semantics and side-effects only on the first successful
initialization.
- Around line 63-65: Py_FinalizeEx currently returns success but doesn't update
initialization state because the code uses a non-resettable Once, so
Py_IsInitialized() stays true; replace the Once-based mechanism with a
resettable atomic state (e.g., an AtomicBool named INITIALIZED) and update all
lifecycle functions: set INITIALIZED.store(true, Ordering::SeqCst) in
Py_InitializeEx (and any init paths), set INITIALIZED.store(false,
Ordering::SeqCst) in Py_FinalizeEx before returning, and have Py_IsInitialized()
read INITIALIZED.load(Ordering::SeqCst); ensure thread-safe Ordering and update
or remove the Once symbol usage so no code still relies on the old Once
primitive.
In `@crates/capi/src/pystate.rs`:
- Around line 36-39: The exported PyEval_SaveThread currently returns null which
breaks the C API contract and will corrupt callers using
Py_BEGIN_ALLOW_THREADS/PyEval_RestoreThread; either implement proper
SaveThread/RestoreThread semantics or stop exporting it. To fix: in
PyEval_SaveThread implement logic that captures the current thread's
PyThreadState (returning a non-null pointer token representing the saved state)
and ensure PyEval_RestoreThread accepts that token to restore thread state,
using the existing PyThreadState type and thread-local VM utilities in
pystate.rs; if you cannot implement correct semantics now, remove the
#[no_mangle]/pub extern "C" export for PyEval_SaveThread (and any paired
PyEval_RestoreThread export) so the unstable API is not exposed. Ensure the
chosen change keeps the stable ABI consistent with callers expecting a valid
non-null PyThreadState pointer.
- Around line 26-34: PyGILState_Ensure currently always returns 0 and
PyGILState_Release is a no-op; implement proper per-thread recursion/state
tracking so each PyGILState_Ensure returns a handle encoding the prior state (or
push the prior state on a per-thread stack in the same thread-local VM storage)
and PyGILState_Release(_state) pops/reads that handle to restore the exact
previous state (including detaching the VM if the matching Ensure attached it);
update the thread-local VM storage to track recursion depth/stack and use those
symbols (PyGILState_Ensure, PyGILState_Release, the existing thread-local VM
storage) when implementing the push/encode-on-Ensure and restore/pop-on-Release
behavior.
In `@crates/capi/src/refcount.rs`:
- Around line 5-17: The exported refcount functions currently accept raw
pointers but are declared callable from safe Rust; change both exports to be
unsafe extern "C" functions and restore the normal no_mangle attribute (replace
#[unsafe(no_mangle)] with #[no_mangle]) so the non-null/valid-pointer contract
is encoded in the signature; update _Py_DecRef and _Py_IncRef signatures to pub
unsafe extern "C" fn _Py_DecRef(op: *mut PyObject) and pub unsafe extern "C" fn
_Py_IncRef(op: *mut PyObject), remove the clippy allow on
not_unsafe_ptr_arg_deref, and adjust the bodies to use NonNull/new or unwrap
patterns consistent with other FFI functions so dereferencing/reconstructing
ownership is only allowed in unsafe code paths.
🪄 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: c1193732-a759-4d95-962e-bf28e2d74df2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/capi/.cargo/config.tomlcrates/capi/Cargo.tomlcrates/capi/pyo3-rustpython.configcrates/capi/src/lib.rscrates/capi/src/pylifecycle.rscrates/capi/src/pystate.rscrates/capi/src/refcount.rs
| #[unsafe(no_mangle)] | ||
| pub extern "C" fn Py_FinalizeEx() -> c_int { | ||
| 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd crates/capi/src && cat -n pylifecycle.rs | head -100Repository: RustPython/RustPython
Length of output: 2517
🏁 Script executed:
rg -n "Py_IsInitialized|Once" crates/capi/src/pylifecycle.rs -B 2 -A 5Repository: RustPython/RustPython
Length of output: 732
🏁 Script executed:
wc -l crates/capi/src/pylifecycle.rsRepository: RustPython/RustPython
Length of output: 98
🏁 Script executed:
cat crates/capi/src/pylifecycle.rsRepository: RustPython/RustPython
Length of output: 2020
🌐 Web query:
CPython Py_IsInitialized Py_FinalizeEx finalization behavior specification
💡 Result:
In CPython, Py_IsInitialized returns true (nonzero) if the Python interpreter has been initialized via Py_Initialize or equivalent, and false (zero) otherwise. After Py_FinalizeEx completes successfully, Py_IsInitialized returns false until the interpreter is reinitialized. Py_IsInitialized can be safely called before initialization or after finalization. Py_FinalizeEx undoes all initializations done by Py_Initialize and related C API calls, destroys undestroyed sub-interpreters, and performs a best-effort cleanup of memory allocated by the interpreter (though extension module memory may not be fully freed). It is a no-op if called when the interpreter is not initialized or called twice without reinitialization in between. It must be called from the main thread with the main interpreter active and the GIL held. It should not be called during Py_RunMain, recursively (e.g., from atexit handlers, object finalizers, or shutdown flushing code), or while non-daemon threads are active. Normally returns 0 on success; returns -1 if errors occur during finalization (e.g., flushing buffered data). Py_Finalize is a void wrapper that ignores the return value. During late-stage finalization (after atexit and non-daemon thread waits), Py_IsFinalizing returns true (added in 3.13), and only the finalization thread can acquire the GIL. Note a known race in 3.13-3.15 where Py_IsInitialized may return true before full initialization completes (e.g., before site.py), though it remains safe to call.
Citations:
- 1: https://docs.python.org/3/c-api/interp-lifecycle.html
- 2: https://docs.python.org/c-api/interp-lifecycle.html
- 3: https://docs.python.org/3.11/c-api/init.html
- 4: https://docs.python.org/3.12/c-api/init.html
Implement proper state tracking to reflect finalization.
Py_FinalizeEx() returns 0 (success) but does not change any state. The current Once primitive cannot be reset, so Py_IsInitialized() will remain true forever. According to the CPython specification, Py_IsInitialized() must return false after Py_FinalizeEx() until the next initialization. Replace the Once mechanism with a mutable state variable (e.g., AtomicBool or similar) that can transition between initialized and finalized states.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/capi/src/pylifecycle.rs` around lines 63 - 65, Py_FinalizeEx currently
returns success but doesn't update initialization state because the code uses a
non-resettable Once, so Py_IsInitialized() stays true; replace the Once-based
mechanism with a resettable atomic state (e.g., an AtomicBool named INITIALIZED)
and update all lifecycle functions: set INITIALIZED.store(true,
Ordering::SeqCst) in Py_InitializeEx (and any init paths), set
INITIALIZED.store(false, Ordering::SeqCst) in Py_FinalizeEx before returning,
and have Py_IsInitialized() read INITIALIZED.load(Ordering::SeqCst); ensure
thread-safe Ordering and update or remove the Once symbol usage so no code still
relies on the old Once primitive.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/capi/src/refcount.rs (1)
5-17:⚠️ Potential issue | 🟠 MajorEncode the raw-pointer contract in the function signatures.
These exports still dereference/reconstruct ownership from
opwhile being callable from safe Rust. Make both functionspub unsafe extern "C" fn, add# Safetydocs, and remove theclippy::not_unsafe_ptr_arg_derefsuppressions; keep the C ABI symbol unchanged.Proposed fix
#[unsafe(no_mangle)] -#[allow(clippy::not_unsafe_ptr_arg_deref)] -pub extern "C" fn _Py_DecRef(op: *mut PyObject) { +/// # Safety +/// +/// `op` must be a valid, non-null pointer to a live `PyObject` with an owned reference. +pub unsafe extern "C" fn _Py_DecRef(op: *mut PyObject) { // By dropping PyObjectRef, we will decrement the reference count. unsafe { PyObjectRef::from_raw(NonNull::new_unchecked(op)) }; } #[unsafe(no_mangle)] -#[allow(clippy::not_unsafe_ptr_arg_deref)] -pub extern "C" fn _Py_IncRef(op: *mut PyObject) { +/// # Safety +/// +/// `op` must be a valid, non-null pointer to a live `PyObject`. +pub unsafe extern "C" fn _Py_IncRef(op: *mut PyObject) { // Don't drop the owned value, as we just want to increment the refcount. core::mem::forget(unsafe { (*op).to_owned() }); }Run this read-only check to verify the signatures and lint suppression are removed:
#!/bin/bash # Description: Verify C-ABI refcount exports encode unsafe raw-pointer contracts. rg -n -C3 '(_Py_DecRef|_Py_IncRef|clippy::not_unsafe_ptr_arg_deref|pub\s+extern\s+"C"\s+fn\s+_Py_)' crates/capi/src/refcount.rsAs per coding guidelines,
**/*.rsshould follow Rust best practices for error handling and memory management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/capi/src/refcount.rs` around lines 5 - 17, Change both exports to encode the raw-pointer contract by making the signatures pub unsafe extern "C" fn _Py_DecRef(op: *mut PyObject) and pub unsafe extern "C" fn _Py_IncRef(op: *mut PyObject), remove the #[allow(clippy::not_unsafe_ptr_arg_deref)] attributes, and keep the no-mangle C ABI symbol. Add concise # Safety doc comments to each function stating the caller must pass a non-null, properly aligned, valid PyObject pointer and that ownership/refcount semantics apply to operations using PyObjectRef::from_raw and (*op).to_owned(). Ensure the body logic (using PyObjectRef::from_raw in _Py_DecRef and core::mem::forget((*op).to_owned()) in _Py_IncRef) remains the same but is now behind an unsafe function signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/capi/Cargo.toml`:
- Line 16: Update the dependency declaration for rustpython-stdlib to be
optional and feature-gated: change the rustpython-stdlib entry so it includes
optional = true and preserves features = ["threading"] (i.e., make
rustpython-stdlib optional), and verify the existing stdlib feature still lists
"rustpython-stdlib" so enabling the stdlib feature pulls in the optional
dependency; adjust the dependency line accordingly and keep the stdlib feature
target unchanged.
---
Duplicate comments:
In `@crates/capi/src/refcount.rs`:
- Around line 5-17: Change both exports to encode the raw-pointer contract by
making the signatures pub unsafe extern "C" fn _Py_DecRef(op: *mut PyObject) and
pub unsafe extern "C" fn _Py_IncRef(op: *mut PyObject), remove the
#[allow(clippy::not_unsafe_ptr_arg_deref)] attributes, and keep the no-mangle C
ABI symbol. Add concise # Safety doc comments to each function stating the
caller must pass a non-null, properly aligned, valid PyObject pointer and that
ownership/refcount semantics apply to operations using PyObjectRef::from_raw and
(*op).to_owned(). Ensure the body logic (using PyObjectRef::from_raw in
_Py_DecRef and core::mem::forget((*op).to_owned()) in _Py_IncRef) remains the
same but is now behind an unsafe function signature.
🪄 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: 5fa614e4-9121-4b67-abb1-14a0faa61559
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.cspell.dict/cpython.txtcrates/capi/.cargo/config.tomlcrates/capi/Cargo.tomlcrates/capi/pyo3-rustpython.configcrates/capi/src/lib.rscrates/capi/src/pylifecycle.rscrates/capi/src/pystate.rscrates/capi/src/refcount.rs
✅ Files skipped from review due to trivial changes (4)
- .cspell.dict/cpython.txt
- crates/capi/pyo3-rustpython.config
- crates/capi/.cargo/config.toml
- crates/capi/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/capi/src/pystate.rs
- crates/capi/src/pylifecycle.rs
|
Please feel free to push to this branch. |
2b4b20f to
63a9c77
Compare
youknowone
left a comment
There was a problem hiding this comment.
good idea to split to small patches 👍
| implementation=CPython | ||
| version=3.14 | ||
| shared=true | ||
| abi3=true |
There was a problem hiding this comment.
is there any freethreading marker? we only target free-treading, which represents abi3t
There was a problem hiding this comment.
abi3t support is not yet implemented in PyO3. But for now I make sure not to use incompatible api's. For progress see PyO3/pyo3#5786
|
@youknowone Can you help me fix clippy? The |
a8cb3b7 to
6a75544
Compare
6a75544 to
7ec3b72
Compare
2691506 to
ce47be6
Compare
Got it to work. |
There was a problem hiding this comment.
Actionable comments posted: 2
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)
2-9:⚠️ Potential issue | 🟠 MajorFeature gate mismatch: strictest re-export and definition gates are incompatible with call-site gates.
Several fork-reinit functions are defined with strict
#[cfg(all(unix, feature = "threading", feature = "host_env"))]gates but called under only#[cfg(feature = "threading")]:
after_fork_child(re-exported at line 2, called at posix.rs:798)reset_weakref_locks_after_fork(defined at object/core.rs:497, called at posix.rs:793)string_pool.reinit_after_fork(defined at intern.rs:40, called at posix.rs:842)codec_registry.reinit_after_fork(defined at codecs.rs:162, called at posix.rs:845)A build with
threadingenabled buthost_envdisabled on Unix will fail compilation because these call sites will resolve to functions that don't exist. Either (a) addfeature = "host_env"to each call-site#[cfg]in posix.rs, or (b) relax the definition gates to require onlyall(unix, feature = "threading")(matching the callers).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/_thread.rs` around lines 2 - 9, The build fails because some fork-reinit functions are defined with #[cfg(all(unix, feature = "threading", feature = "host_env"))] but are called under only #[cfg(feature = "threading")]; update the cfgs so callers and definitions match: relax the definition gates for after_fork_child, reset_weakref_locks_after_fork, the string_pool.reinit_after_fork method, and codec_registry.reinit_after_fork to use #[cfg(all(unix, feature = "threading"))] (i.e., remove the feature = "host_env" requirement) so the symbols exist when threading is enabled, or alternatively add feature = "host_env" to the callers' cfgs in posix.rs—pick one consistent approach and apply it to all listed symbols.
♻️ Duplicate comments (1)
crates/capi/src/pylifecycle.rs (1)
7-7:⚠️ Potential issue | 🟠 MajorTrack logical finalization separately from the
OnceLock.
Py_FinalizeEx()reports success but leavesMAIN_INTERPpopulated, soPy_IsInitialized()remains true forever after the firstPy_InitializeEx(). If full teardown is deferred, at least keep a resettable lifecycle flag so callers can observefinalize → uninitialized → initializecorrectly.Minimal state-tracking direction
use crate::pystate::attach_vm_to_thread; use core::ffi::c_int; use rustpython_vm::Interpreter; use rustpython_vm::vm::thread::ThreadedVirtualMachine; -use std::sync::{Mutex, OnceLock}; +use std::sync::{ + atomic::{AtomicBool, Ordering}, + Mutex, OnceLock, +}; static MAIN_INTERP: OnceLock<Mutex<Interpreter>> = OnceLock::new(); +static INITIALIZED: AtomicBool = AtomicBool::new(false); @@ pub extern "C" fn Py_IsInitialized() -> c_int { - MAIN_INTERP.get().is_some() as c_int + INITIALIZED.load(Ordering::SeqCst) as c_int } @@ pub extern "C" fn Py_InitializeEx(_initsigs: c_int) { MAIN_INTERP.get_or_init(|| Interpreter::with_init(Default::default(), |_vm| {}).into()); + INITIALIZED.store(true, Ordering::SeqCst); attach_vm_to_thread(); } @@ pub extern "C" fn Py_FinalizeEx() -> c_int { + INITIALIZED.store(false, Ordering::SeqCst); 0 }Also applies to: 20-21, 42-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/capi/src/pylifecycle.rs` at line 7, MAIN_INTERP (a OnceLock<Mutex<Interpreter>>) is never cleared so Py_IsInitialized() stays true after Py_FinalizeEx(); introduce a separate resettable lifecycle flag (e.g., an AtomicBool like PY_INITIALIZED) and update it to true in Py_InitializeEx and false in Py_FinalizeEx, then have Py_IsInitialized read that flag instead of the OnceLock; keep MAIN_INTERP as-is (do not rely on dropping it) so callers see a correct initialized/finalized lifecycle without changing the OnceLock usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/_imp.rs`:
- Around line 79-96: Update the call sites that invoke the fork helper functions
so their cfg gating matches the exports: wrap the calls to
acquire_imp_lock_for_fork(), after_fork_child_imp_lock_release(),
reinit_imp_lock_after_fork(), and release_imp_lock_after_fork_parent() with
cfg(all(unix, feature = "threading", feature = "host_env")) so they are only
compiled when those functions are exported; locate the call sites where those
four symbols are used and replace the existing cfg checks (e.g., feature =
"threading" or all(unix, feature = "threading")) with the new all(unix, feature
= "threading", feature = "host_env") gate.
In `@crates/vm/src/stdlib/_io.rs`:
- Around line 5-6: The call-site for reinit_std_streams_after_fork in posix.rs
is only guarded by #[cfg(feature = "threading")] but the symbol is exported in
_io.rs under #[cfg(all(unix, feature = "threading", feature = "host_env"))];
change the cfg on the posix.rs invocation to #[cfg(all(unix, feature =
"threading", feature = "host_env"))] so the call is only compiled when the
function is available (search for the reinit_std_streams_after_fork call in
posix.rs to update its cfg).
---
Outside diff comments:
In `@crates/vm/src/stdlib/_thread.rs`:
- Around line 2-9: The build fails because some fork-reinit functions are
defined with #[cfg(all(unix, feature = "threading", feature = "host_env"))] but
are called under only #[cfg(feature = "threading")]; update the cfgs so callers
and definitions match: relax the definition gates for after_fork_child,
reset_weakref_locks_after_fork, the string_pool.reinit_after_fork method, and
codec_registry.reinit_after_fork to use #[cfg(all(unix, feature = "threading"))]
(i.e., remove the feature = "host_env" requirement) so the symbols exist when
threading is enabled, or alternatively add feature = "host_env" to the callers'
cfgs in posix.rs—pick one consistent approach and apply it to all listed
symbols.
---
Duplicate comments:
In `@crates/capi/src/pylifecycle.rs`:
- Line 7: MAIN_INTERP (a OnceLock<Mutex<Interpreter>>) is never cleared so
Py_IsInitialized() stays true after Py_FinalizeEx(); introduce a separate
resettable lifecycle flag (e.g., an AtomicBool like PY_INITIALIZED) and update
it to true in Py_InitializeEx and false in Py_FinalizeEx, then have
Py_IsInitialized read that flag instead of the OnceLock; keep MAIN_INTERP as-is
(do not rely on dropping it) so callers see a correct initialized/finalized
lifecycle without changing the OnceLock usage.
🪄 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: c5ea4489-fdf4-4945-bd15-c2b5f021848c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.cspell.dict/cpython.txtcrates/capi/.cargo/config.tomlcrates/capi/Cargo.tomlcrates/capi/pyo3-rustpython.configcrates/capi/src/lib.rscrates/capi/src/pylifecycle.rscrates/capi/src/pystate.rscrates/capi/src/refcount.rscrates/vm/src/codecs.rscrates/vm/src/intern.rscrates/vm/src/object/core.rscrates/vm/src/stdlib/_imp.rscrates/vm/src/stdlib/_io.rscrates/vm/src/stdlib/_thread.rscrates/vm/src/vm/thread.rs
✅ Files skipped from review due to trivial changes (6)
- crates/vm/src/vm/thread.rs
- crates/capi/.cargo/config.toml
- crates/capi/pyo3-rustpython.config
- crates/capi/src/lib.rs
- crates/capi/src/refcount.rs
- crates/capi/src/pystate.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .cspell.dict/cpython.txt
- crates/capi/Cargo.toml
| #[cfg(all(unix, feature = "threading", feature = "host_env"))] | ||
| pub(crate) fn acquire_imp_lock_for_fork() { | ||
| lock::acquire_lock_for_fork(); | ||
| } | ||
|
|
||
| #[cfg(feature = "threading")] | ||
| #[cfg(all(unix, feature = "threading", feature = "host_env"))] | ||
| pub(crate) fn release_imp_lock_after_fork_parent() { | ||
| lock::release_lock_after_fork_parent(); | ||
| } | ||
|
|
||
| #[cfg(all(unix, feature = "threading"))] | ||
| #[cfg(all(unix, feature = "threading", feature = "host_env"))] | ||
| pub(crate) unsafe fn reinit_imp_lock_after_fork() { | ||
| unsafe { lock::reinit_after_fork() } | ||
| } | ||
|
|
||
| #[cfg(all(unix, feature = "threading"))] | ||
| #[cfg(all(unix, feature = "threading", feature = "host_env"))] | ||
| pub(crate) unsafe fn after_fork_child_imp_lock_release() { | ||
| unsafe { lock::after_fork_child_reinit_and_release() } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect import-lock after-fork helper call sites and nearby cfg guards.
rg -n -C4 'acquire_imp_lock_for_fork|release_imp_lock_after_fork_parent|reinit_imp_lock_after_fork|after_fork_child_imp_lock_release' --type rustRepository: RustPython/RustPython
Length of output: 4130
🏁 Script executed:
#!/bin/bash
# Check the full context around line 851 in posix.rs to see the cfg guard
sed -n '840,860p' crates/vm/src/stdlib/posix.rs | cat -nRepository: RustPython/RustPython
Length of output: 909
🏁 Script executed:
#!/bin/bash
# Check if _imp.rs has conditional no-op exports for when host_env is unavailable
sed -n '79,110p' crates/vm/src/stdlib/_imp.rs | cat -nRepository: RustPython/RustPython
Length of output: 1301
🏁 Script executed:
#!/bin/bash
# Find the function containing line 851 in posix.rs to check parent cfg guard
sed -n '830,855p' crates/vm/src/stdlib/posix.rs | cat -nRepository: RustPython/RustPython
Length of output: 1315
🏁 Script executed:
#!/bin/bash
# Also check if there are any conditional stubs or no-op exports for when host_env is not available
rg -A5 'cfg.*host_env' crates/vm/src/stdlib/_imp.rsRepository: RustPython/RustPython
Length of output: 1583
🏁 Script executed:
#!/bin/bash
# Find the function definition that contains line 851
sed -n '800,860p' crates/vm/src/stdlib/posix.rs | grep -n "fn " | head -5Repository: RustPython/RustPython
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Get full function signature and cfg around line 851
sed -n '795,860p' crates/vm/src/stdlib/posix.rs | cat -nRepository: RustPython/RustPython
Length of output: 3390
🏁 Script executed:
#!/bin/bash
# Check if host_env is a conditional feature that can be disabled
rg -n "feature.*host_env|host_env.*feature" crates/vm/Cargo.toml crates/Cargo.tomlRepository: RustPython/RustPython
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Check the full context of call sites to see exact cfg guards
sed -n '760,770p' crates/vm/src/stdlib/posix.rs | cat -n
sed -n '800,810p' crates/vm/src/stdlib/posix.rs | cat -n
sed -n '855,865p' crates/vm/src/stdlib/posix.rs | cat -nRepository: RustPython/RustPython
Length of output: 1543
🏁 Script executed:
#!/bin/bash
# Check for host_env feature definition in the workspace
find . -name "Cargo.toml" -type f | xargs grep -l "host_env" | head -5Repository: RustPython/RustPython
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Check the root Cargo.toml for feature definitions
sed -n '/\[features\]/,/^\[/p' Cargo.toml | head -30Repository: RustPython/RustPython
Length of output: 963
🏁 Script executed:
#!/bin/bash
# Check if there are any conditional no-op stubs for the fork helpers when host_env is unavailable
rg -B5 -A5 'cfg.*not.*host_env' crates/vm/src/stdlib/_imp.rsRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Confirm the exact cfg guard on the reinit_locks_after_fork function
sed -n '818,835p' crates/vm/src/stdlib/posix.rs | cat -nRepository: RustPython/RustPython
Length of output: 1086
🏁 Script executed:
#!/bin/bash
# Check if there are any conditional stubs anywhere for these functions when host_env is unavailable
rg 'acquire_imp_lock_for_fork|release_imp_lock_after_fork_parent|reinit_imp_lock_after_fork|after_fork_child_imp_lock_release' crates/vm/src/stdlib/_imp.rsRepository: RustPython/RustPython
Length of output: 272
🏁 Script executed:
#!/bin/bash
# Verify: when host_env feature is off, are these symbols completely unavailable?
# Check what happens after line 97 in _imp.rs (after the host_env gated functions)
sed -n '97,120p' crates/vm/src/stdlib/_imp.rs | cat -nRepository: RustPython/RustPython
Length of output: 1005
Align call sites with the new export cfg gates to prevent linker errors.
The four fork helpers in _imp.rs (lines 79–96) are now gated with all(unix, feature = "threading", feature = "host_env"), but their call sites in posix.rs only check feature = "threading" or all(unix, feature = "threading"). When host_env is disabled, these functions will not be exported, causing linker failures.
Update the four call sites in posix.rs to require all(unix, feature = "threading", feature = "host_env"):
- Line 763:
acquire_imp_lock_for_fork() - Line 804:
after_fork_child_imp_lock_release() - Line 851:
reinit_imp_lock_after_fork()(withinreinit_locks_after_forkfunction) - Line 860:
release_imp_lock_after_fork_parent()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/_imp.rs` around lines 79 - 96, Update the call sites
that invoke the fork helper functions so their cfg gating matches the exports:
wrap the calls to acquire_imp_lock_for_fork(),
after_fork_child_imp_lock_release(), reinit_imp_lock_after_fork(), and
release_imp_lock_after_fork_parent() with cfg(all(unix, feature = "threading",
feature = "host_env")) so they are only compiled when those functions are
exported; locate the call sites where those four symbols are used and replace
the existing cfg checks (e.g., feature = "threading" or all(unix, feature =
"threading")) with the new all(unix, feature = "threading", feature =
"host_env") gate.
There was a problem hiding this comment.
posix.rs already requires unix so that clause is redundant right?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the top-level cfg gate on posix.rs or any module-level attribute
head -20 crates/vm/src/stdlib/posix.rs
# Check the cfg guards on the exact call sites
sed -n '758,770p' crates/vm/src/stdlib/posix.rs | cat -n
sed -n '800,812p' crates/vm/src/stdlib/posix.rs | cat -n
sed -n '848,865p' crates/vm/src/stdlib/posix.rs | cat -n
Length of output: 2403
@bschoenmaeckers — you're right, within posix.rs the unix clause is redundant since the module is inherently Unix-only. My apologies for the noise there.
That said, the core concern still stands: the four call sites currently only gate on feature = "threading", but the exported functions in _imp.rs now additionally require feature = "host_env". When threading is enabled but host_env is not, the call sites will reference symbols that don't exist, causing a compile error.
The fix is straightforward — just add feature = "host_env" to the existing guards in posix.rs (no need to add unix):
// Line ~763
#[cfg(all(feature = "threading", feature = "host_env"))]
crate::stdlib::_imp::acquire_imp_lock_for_fork();
// Line ~804
#[cfg(all(feature = "threading", feature = "host_env"))]
unsafe {
crate::stdlib::_imp::after_fork_child_imp_lock_release()
};
// Line ~851 (inside the reinit_locks_after_fork unsafe block)
#[cfg(all(feature = "threading", feature = "host_env"))]
crate::stdlib::_imp::reinit_imp_lock_after_fork();
// Line ~860
#[cfg(all(feature = "threading", feature = "host_env"))]
crate::stdlib::_imp::release_imp_lock_after_fork_parent();🧠 Learnings used
Learnt from: joshuamegnauth54
Repo: RustPython/RustPython PR: 7646
File: crates/vm/src/stdlib/_thread.rs:0-0
Timestamp: 2026-04-21T18:46:19.577Z
Learning: In RustPython's `crates/vm/src/stdlib/_thread.rs`, the `thread_to_id` function on Unix casts `handle.as_pthread_t()` (which returns `RawPthread`, a platform-dependent alias) using `as _` rather than `as u64`. This is intentional: on Linux `RawPthread` is already `u64`, so `as u64` triggers Clippy's `trivial_numeric_casts` lint; `as _` lets the compiler infer the target type from the `u64` return type, satisfying portability needs on macOS/BSD without suppressing Clippy.
Learnt from: arihant2math
Repo: RustPython/RustPython PR: 5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
Learnt from: youknowone
Repo: RustPython/RustPython PR: 7333
File: crates/vm/src/gc_state.rs:833-842
Timestamp: 2026-03-04T11:07:39.411Z
Learning: In RustPython's `crates/vm/src/gc_state.rs`, the `gc_state()` function intentionally uses `rustpython_common::static_cell!` which becomes thread-local when the "threading" feature is disabled. This is correct by design: in non-threading mode, `PyRwLock`/`PyMutex` use `Cell`-based locks that are `!Sync` (preventing a process-wide global `GcState` from compiling), and reference counting uses `Rc` instead of `Arc`, so Python objects cannot cross OS thread boundaries. Each thread therefore correctly owns and GC-tracks only its own objects. Do not flag this as a bug.
Learnt from: bschoenmaeckers
Repo: RustPython/RustPython PR: 7648
File: crates/capi/src/pystate.rs:36-39
Timestamp: 2026-04-21T13:48:35.041Z
Learning: In RustPython's `crates/capi/src/pystate.rs`, `PyEval_SaveThread` intentionally returns `null` because RustPython has no GIL; `Py_BEGIN_ALLOW_THREADS`/`Py_END_ALLOW_THREADS` are no-ops by design. Do not flag the null return as a contract violation. Note: `PyEval_RestoreThread` still needs a corresponding no-op stub export to satisfy callers using the full macro pair.
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using `#ifdef` checks rather than providing fallback values for other platforms.
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 7181
File: crates/stdlib/src/csv.rs:20-20
Timestamp: 2026-02-17T07:54:36.200Z
Learning: In RustPython, ensure that modules using Python sequence-like types bring in extension methods by importing the trait (for example, 'use rustpython_vm::sliceable::SliceableSequenceOp;'). Even if the trait name isn't directly referenced in code, its methods (such as len()) become in-scope on types like PyStr. Add this import in modules that rely on those extension methods to avoid compilation errors.
| #[cfg(all(unix, feature = "threading", feature = "host_env"))] | ||
| pub(crate) use _io::reinit_std_streams_after_fork; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect after-fork stdio reinit call sites and their cfg guards.
rg -n -C4 'reinit_std_streams_after_fork' --type rustRepository: RustPython/RustPython
Length of output: 2102
Sync the call-site cfg in posix.rs with the tightened export cfg in _io.rs.
reinit_std_streams_after_fork is only compiled for all(unix, feature = "threading", feature = "host_env"), but the call at crates/vm/src/stdlib/posix.rs:784 is guarded by only #[cfg(feature = "threading")]. Builds with threading enabled and host_env disabled will fail to resolve this symbol.
Update the cfg on line 782 to match the function's availability:
Fix in crates/vm/src/stdlib/posix.rs
- #[cfg(feature = "threading")]
+ #[cfg(all(unix, feature = "threading", feature = "host_env"))]
unsafe {
crate::stdlib::_io::reinit_std_streams_after_fork(vm)
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/_io.rs` around lines 5 - 6, The call-site for
reinit_std_streams_after_fork in posix.rs is only guarded by #[cfg(feature =
"threading")] but the symbol is exported in _io.rs under #[cfg(all(unix, feature
= "threading", feature = "host_env"))]; change the cfg on the posix.rs
invocation to #[cfg(all(unix, feature = "threading", feature = "host_env"))] so
the call is only compiled when the function is available (search for the
reinit_std_streams_after_fork call in posix.rs to update its cfg).
This add the most basic c api I can think off. This way we can iterate over how we want to handle the interpreter initialization api's. After this, the road is clear to implement most of the ABI3 api's one-by-one.
This also include my basic pyo3 test harness that exposes the c api's in a safe and battle tested way. To run the tests use
cargo test -p rustpython-capi.References:
xref #7562
Summary by CodeRabbit
New Features
Chores
Behavioral Changes