Skip to content

Release deque items outside locks during clear and assignment - #8875

Merged
youknowone merged 1 commit into
RustPython:mainfrom
1ndahous3:deque_release_refs
Sep 28, 2026
Merged

Release deque items outside locks during clear and assignment#8875
youknowone merged 1 commit into
RustPython:mainfrom
1ndahous3:deque_release_refs

Conversation

@1ndahous3

@1ndahous3 1ndahous3 commented Sep 27, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Prevent deadlocks when finalizers access the same deque during clear, indexed replacement, or failed assignment.
  • Detach the queue before releasing its old elements, freeing its old storage and preserving items appended by finalizers.
  • Preserve existing iterators when clearing an empty deque.

Follows up on #8847 and #8860, covering the remaining clear and indexed-assignment release paths.

AI assistance

Written with Codex (GPT-6), reviewed by a human before submission.

Summary by CodeRabbit

  • Bug Fixes
    • Deque operations now release their lock before discarded items are finalized, avoiding lock-related issues.
    • Clearing a non-empty deque continues to invalidate existing iterators; clearing an empty deque leaves it unchanged.
    • Failed assignments to an empty deque still raise an index error.

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml

Review profile: CHILL

Plan: Advanced

Run ID: 1278c8e0-8f15-4897-8788-2725f7c151e7

📥 Commits

Reviewing files that changed from the base of the PR and between 908368e and 95fd656.

📒 Files selected for processing (2)
  • crates/vm/src/stdlib/_collections.rs
  • extra_tests/snippets/stdlib_collections_deque.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Walkthrough

Deque clearing and item assignment now release the lock before removed or replaced items are finalized. The changes also define empty-deque clearing behavior and add tests for iterator state, finalizer effects, and failed assignment.

Changes

Deque mutation and finalization

Layer / File(s) Summary
Deque mutation and finalizer handling
crates/vm/src/stdlib/_collections.rs, extra_tests/snippets/stdlib_collections_deque.py		 clear() leaves an empty deque unchanged. For a nonempty deque, it takes the contents while locked and drops them after unlocking. __setitem__ drops the replaced item after unlocking; invalid indices still raise IndexError. Tests cover iterator state and finalizer behavior, including failed assignment to an empty deque.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: youknowone

Merge Risk: ⚪ Minimal · up to 95fd6

No actionable issue remains; the PR is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 95fd6

The change addresses a deadlock risk without adding a new entrypoint or privilege boundary. Finalizer timing is observable, but no introduced security issue was established. Concurrent iterator behavior remains uncertain.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The evidenced effect is confined to the deque object being mutated and code run by its removed or rejected objects’ finalizers; broader tenant, service, or datastore exposure is not established.

Trust Boundaries and Controls

  • observed — The existing deque write lock still protects the storage mutation. Finalizers run only after that mutation and lock scope complete; no new dispatch entrypoint is shown.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: releasing deque items outside locks during clear and assignment to prevent deadlocks.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@youknowone
youknowone merged commit bb054f6 into RustPython:main Sep 28, 2026
21 checks passed
@1ndahous3
1ndahous3 deleted the deque_release_refs branch September 28, 2026 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@1ndahous3 @youknowone