Skip to content

fix pyexpat hang #6507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
youknowone merged 1 commit into from
Dec 25, 2025
Merged

fix pyexpat hang #6507

youknowone merged 1 commit into from
Dec 25, 2025

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Dec 25, 2025 •
edited by coderabbitai bot
Loading

Fix #6504

Summary by CodeRabbit

  • Bug Fixes
    • Improved stability of event handler invocation to prevent potential concurrency-related issues.

✏️ Tip: You can customize this high-level summary in your review settings.

@youknowone youknowone marked this pull request as ready for review December 25, 2025 03:43
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 25, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

The invoke_handler function in pyexpat.rs has been reworked to clone the handler under the read lock and invoke it after releasing the lock, rather than holding the lock during handler invocation. This lock-release-before-invocation pattern mitigates potential deadlock and re-entrancy issues.

Changes

Cohort / File(s) Summary
Lock management refactor
crates/stdlib/src/pyexpat.rs		 Modified invoke_handler to clone handler under read lock, then invoke on cloned handle after lock release, preventing deadlock scenarios during handler execution.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A lock held too long causes trouble, you see,
So we clone and release before running spree,
No deadlock shall haunt our expat's invite,
The handler runs free—now prompt-toolkit's bright! ✨

Pre-merge checks and finishing touches

✅ 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 'fix pyexpat hang' is directly related to the main change: reworking invoke_handler to prevent deadlock/re-entrant issues that cause hanging behavior.
Linked Issues check ✅ Passed The PR addresses the hanging issue in prompt-toolkit by fixing deadlock potential in pyexpat's invoke_handler through lock management refactoring.
Out of Scope Changes check ✅ Passed All changes are scoped to pyexpat.rs and directly address the deadlock/re-entrant hanging issue in linked issue #6504.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aae6bf5 and 25a25b7.

📒 Files selected for processing (1)
  • crates/stdlib/src/pyexpat.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to 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/stdlib/src/pyexpat.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 rust tests (macos-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Ensure compilation on various targets
  • GitHub Check: Check Rust code with clippy
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run tests under miri
🔇 Additional comments (1)
crates/stdlib/src/pyexpat.rs (1)

100-108: Excellent fix to prevent deadlock during handler invocation.

The lock-release-before-callback pattern is correct and follows Rust best practices. By cloning the handler under the read lock and then invoking it after the lock is released, you prevent potential deadlocks that would occur if the handler tries to access parser state or re-enters the parser.

Cloning PyObjectRef (which is Arc-based) is cheap—just a reference count increment—so there's no performance concern.

The .ok() on line 107 is appropriate for event-driven parsing, where handler exceptions should not crash the parser. This allows parsing to continue despite handler errors, which matches the typical behavior of XML parsers.

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 and usage tips.

@youknowone youknowone merged commit 92acf33 into RustPython:main Dec 25, 2025
13 checks passed
@youknowone youknowone deleted the pyexpat branch December 25, 2025 04:31
@coderabbitai coderabbitai bot mentioned this pull request Dec 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

prompt-toolkit hanging

1 participant

@youknowone