fix pyexpat hang by youknowone · Pull Request #6507 · RustPython/RustPython

youknowone

📝 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.}