Align del behavior#6772
Conversation
📝 WalkthroughWalkthroughRemoved specific multiprocessing Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DropLogic as drop_slow_inner
participant RefCount as RefCount
participant Object as Obj
participant Finalizer as __del__
DropLogic->>RefCount: inc_by(2) (temporary resurrect)
DropLogic->>Finalizer: call_slot_del(Object)
Finalizer->>Object: run __del__
Note right of Object: __del__ may create new refs (resurrect)
Finalizer-->>DropLogic: return
DropLogic->>RefCount: dec() (original)
DropLogic->>RefCount: dec() (post-finalizer)
DropLogic->>DropLogic: compare strong_count to detect resurrection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Sorry, something went wrong.
5ae9172 to
378f737
Compare
January 30, 2026 23:13
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@crates/common/src/refcount.rs`:
- Around line 39-44: The inc_by method must validate its n argument before using
it to detect overflow; add an explicit bounds check at the top of pub fn
inc_by(&self, n: usize) so that if n == 0 it returns early and if n > Self::MASK
it aborts (to avoid subtraction underflow when computing Self::MASK - n). After
that, keep the existing fetch_add/overflow check (using self.strong.fetch_add)
unchanged; this ensures the existing overflow detection logic remains correct
for valid n values (<= MASK).
In `@crates/vm/src/object/core.rs`:
- Around line 223-227: When reattaching an object in the block that checks `if
inner.obj.is_none() { inner.obj = Some(NonNull::from(obj)); }`, also restore the
weakref invariant by incrementing `inner.ref_count` to reflect the "weakrefs + 1
if alive" ownership; i.e., when you set `inner.obj = Some(...)` increment
`inner.ref_count` (using the same semantics as `clear()` decremented it — e.g.,
checked_add or the crate's existing ref-count increment helper) so the list
won't be dropped while the object is alive and avoid overflow/underflow by
matching the existing ref-count handling used elsewhere (e.g., the `clear()` and
ref-count manipulation functions).
- Around line 843-867: The destructor path increments temporary refs via
zelf.0.ref_count.inc_by(2) then on a resurrection only performs one decrement,
leaking one temporary ref; change the logic so both temporary decs always run
(call zelf.0.ref_count.dec() twice) and use the return/result of the second dec
to determine whether references remain (i.e., whether to treat the object as
still alive) instead of skipping the second dec on resurrection; update the
block around strong_count(), slot_del(zelf, vm), and the two ref_count.dec()
calls (and the resurrection check that currently uses after_del > after_inc) so
the second dec's result drives the decision.
Sorry, something went wrong.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] lib: cpython/Lib/hmac.py dependencies:
dependent tests: (4 tests)
[ ] lib: cpython/Lib/multiprocessing dependencies:
dependent tests: (5 tests)
Legend:
|
Sorry, something went wrong.
fanninpm
left a comment
There was a problem hiding this comment.
I do not have write access to the repository, so I cannot approve a PR to clear it for merging.
Sorry, something went wrong.
ba8749b
into
RustPython:main
Jan 31, 2026
fanninpm
left a comment
There was a problem hiding this comment.
Looks good to me, aside from the one thing the AI brought up.
Sorry, something went wrong.
Thanks! |
Sorry, something went wrong.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.