Fix potential deadlocks #6509
Fix potential deadlocks #6509
Conversation
📝 WalkthroughWalkthroughThree builtin and stdlib modules refactored to prevent deadlocks by cloning callable objects and values under read locks, then releasing locks before invoking Python code. Changes affect classmethod descriptor access, property getter/setter/deleter invocation, and partial function callable/representation logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
8276cf6 to
36fd0b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/vm/src/builtins/classmethod.rs (2)
142-158: Remaining potential deadlock: lock held duringget_attrcalls.These getters still hold the lock while calling Python code, which could cause the same deadlock this PR aims to fix:
// Line 143 - lock held during get_attr self.callable.lock().get_attr("__module__", vm)Consider applying the same clone-and-release pattern:
🔎 Proposed fix
#[pygetset] fn __module__(&self, vm: &VirtualMachine) -> PyResult { - self.callable.lock().get_attr("__module__", vm) + let callable = self.callable.lock().clone(); + callable.get_attr("__module__", vm) } #[pygetset] fn __qualname__(&self, vm: &VirtualMachine) -> PyResult { - self.callable.lock().get_attr("__qualname__", vm) + let callable = self.callable.lock().clone(); + callable.get_attr("__qualname__", vm) } #[pygetset] fn __name__(&self, vm: &VirtualMachine) -> PyResult { - self.callable.lock().get_attr("__name__", vm) + let callable = self.callable.lock().clone(); + callable.get_attr("__name__", vm) } #[pygetset] fn __annotations__(&self, vm: &VirtualMachine) -> PyResult { - self.callable.lock().get_attr("__annotations__", vm) + let callable = self.callable.lock().clone(); + callable.get_attr("__annotations__", vm) }
169-175: Remaining potential deadlock: lock held duringset_attr.The setter holds the lock while calling
set_attr, which can invoke Python code.🔎 Proposed fix
#[pygetset(setter)] fn set___isabstractmethod__(&self, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { - self.callable - .lock() - .set_attr("__isabstractmethod__", value, vm)?; + let callable = self.callable.lock().clone(); + callable.set_attr("__isabstractmethod__", value, vm)?; Ok(()) }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/vm/src/builtins/classmethod.rscrates/vm/src/builtins/property.rscrates/vm/src/stdlib/functools.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 Rust 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/builtins/classmethod.rscrates/vm/src/stdlib/functools.rscrates/vm/src/builtins/property.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
🔇 Additional comments (7)
crates/vm/src/builtins/classmethod.rs (1)
60-66: LGTM on the deadlock fix pattern.The clone-and-release pattern correctly prevents deadlocks by ensuring no lock is held when invoking Python code (
get_attr,call).crates/vm/src/stdlib/functools.rs (2)
250-281: LGTM on the deadlock fix incall.The scoped block pattern ensures the read lock is released before any Python code executes (
func.call). The cloned values are safely used after lock release.
286-320: LGTM on the deadlock fix inrepr_str.Same correct clone-and-release pattern applied to the repr logic, ensuring locks are released before calling
repr(vm)on func, args, and keyword values.crates/vm/src/builtins/property.rs (4)
58-64: LGTM on the deadlock fix indescr_get.Correctly clones the getter while holding the read lock (via the
if let Some(getter) = zelf.getter.read().clone()pattern), releases the lock, then calls the getter.
74-81: LGTM on the deadlock fix inget_property_name.Both the
namefield andgetterare cloned before any Python code (get_attr) is invoked.
108-126: LGTM on the deadlock fix indescr_set.Both the
setteranddeleterpaths correctly clone under lock before calling Python code.
279-311: LGTM on the deadlock fixes in__isabstractmethod__accessors.Both the getter and setter correctly clone the relevant fields before invoking Python code (
get_attr,try_to_bool,set_attr).
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.