Visitar URL original
Release interpreter channel payloads outside internal locks by 1ndahous3 · Pull Request #8964 · RustPython/RustPython · GitHub
Skip to content

Release interpreter channel payloads outside internal locks - #8964

Merged
youknowone merged 1 commit into
RustPython:mainfrom
1ndahous3:channel_release_locks
Oct 5, 2026
Merged

Release interpreter channel payloads outside internal locks#8964
youknowone merged 1 commit into
RustPython:mainfrom
1ndahous3:channel_release_locks

Conversation

@1ndahous3

@1ndahous3 1ndahous3 commented Oct 4, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Release queued payloads after unlocking the channel directory and state during destroy, close, ChannelID disposal, interpreter cleanup, and send timeout. Buffer finalizers can reenter the channel API without deadlocking.
  • Snapshot interpreter IDs before locking channel state, and create Python results and exceptions after releasing that lock.

Extracted from #8944 as an independent fix.

AI assistance

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

Summary by CodeRabbit

  • Bug Fixes
    • Improved channel cleanup reliability when releasing queued buffers triggers code that inspects channels. This applies during channel destruction, forced close, reference release, interpreter cleanup, and timed-out sends.
    • Timed-out sends continue to raise TimeoutError, and queued buffer finalizers run once.
  • Tests
    • Added regression coverage for channel cleanup and reentrant buffer finalizers.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 4, 2026

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai

coderabbitai Bot commented Oct 4, 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.

🧰 Additional context used
📚 Code guidelines (1) 
AGENTS.md — auto-discovered

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: f0b63e8f-48bf-4701-9302-a672749699ed
📥 Commits

Reviewing files that changed from the base of the PR and between 7536730 and 9154f7e.

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

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

📝 Walkthrough

Walkthrough

Channel operations now defer releasing queued values until after channel state locks are released. Interpreter listing also snapshots runtime data before locking channel state. A subprocess regression test checks finalizer reentry during channel teardown and timed-out sends.

Changes

Channel item retirement

Layer / File(s) Summary
Channel teardown and item retirement
crates/vm/src/stdlib/_interpchannels.rs		 Channel destruction, final channel-reference release, and forced close detach queued items and retire them after releasing locks.
Interpreter cleanup and listing
crates/vm/src/stdlib/_interpchannels.rs		 Interpreter cleanup defers dropping shared values. list_interpreters obtains runtime interpreter data before locking channel state and releases the lock before returning.
Timed-send retirement and regression coverage
crates/vm/src/stdlib/_interpchannels.rs, extra_tests/snippets/stdlib_subinterpreters.py		 Timed-out sends retain the removed item until channel state is unlocked. The subprocess test checks finalizer reentry during teardown, forced close, channel-reference release, and timeout.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Suggested reviewers: youknowone

Merge Risk: ⚪ Minimal · up to 9154f

This change moves the release of queued channel payloads outside internal locks so finalizers can safely call back into the channel API. No concrete merge-blocking risk was found in the supplied context, and a regression test covers the affected paths.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 9154f

The change reduces deadlock risk without an identified increase in access or privileges. Residual risk is low because concurrent cleanup and listing behavior are not fully specified or covered.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated failure domain is channel operations and shared payloads within a process-wide interpreter context. The supplied evidence does not establish tenant, service, credential, or deployment-level exposure.

Security Findings and Attack Paths

  • inferred — A caller-supplied buffer owner can execute a finalizer during payload retirement. The change removes the demonstrated lock-held reentry hazard; inspected reentrant paths do not establish a new identity bypass or privilege gain.

Trust Boundaries and Controls

  • observed — Public destroy and close still resolve the supplied CID before invoking lifecycle helpers. Sending retains channel lookup, closing-state checks, and interpreter association. Unlocking retirement does not replace these checks with caller-provided interpreter identity.

Resilience and Maintainability Implications

  • inferred — Queue removal remains serialized, timeout cancellation matches waiter identity, and waiter release preserves the first terminal result while notifying waiters. These controls support failure containment under competing receive and teardown paths, although the conclusion is source-based rather than a completed concurrency test.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: releasing interpreter channel payloads after internal locks are released.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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 approved these changes Oct 5, 2026
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@youknowone
youknowone merged commit e5f332f into RustPython:main Oct 5, 2026
21 checks passed
@1ndahous3
1ndahous3 deleted the channel_release_locks branch October 9, 2026 14:04
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