GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — minimize ssl lock by youknowone · Pull Request #6376 · RustPython/RustPython

Conversation

youknowone commented Dec 9, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Summary by CodeRabbit

Release Notes

Refactor
- Restructured SSL/TLS certificate loading and CRL handling to reduce lock contention and improve performance during secure operations.
- Optimized secure connection state operations to occur outside critical sections, reducing blocking and improving responsiveness for TLS transactions.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone marked this pull request as ready for review

December 9, 2025 13:10

coderabbitai Bot commented Dec 9, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Warning

Rate limit exceeded

@youknowone has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 0 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between d514dc7 and 0f5ffbd.

📒 Files selected for processing (1)

crates/stdlib/src/ssl.rs (7 hunks)

Walkthrough

The PR refactors SSL socket certificate and CRL loading logic in crates/stdlib/src/ssl.rs to reduce lock contention. Argument parsing and certificate validation occur before lock acquisition, heavy parsing operations are moved outside critical sections, and CRL handling is separated from root store mutations.

Changes

Cohort / File(s)	Summary
SSL lock contention reduction `crates/stdlib/src/ssl.rs`	Reordered argument parsing for SSL loading paths to occur before acquiring locks. Moved X509 certificate parsing outside critical sections. Separated CRL handling from root/store mutations. Refactored SNI accessors, get_peercert, cipher/version methods, and session tracking to minimize lock hold duration and avoid nested locks. Preserved existing error propagation and exception handling.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Lock correctness and deadlock prevention: Verify that moving operations outside locks doesn't introduce data races or deadlocks, particularly around SNI state cloning and nested lock avoidance
Certificate/CRL parsing correctness: Ensure pre-validation and deferred mutation of root_store/ca_certs_der don't break certificate chain validation or CRL verification
get_peercert refactoring: Confirm peer certificate extraction outside locks maintains consistency and compatibility with existing return types
Error handling paths: Validate that errno preservation and SSL error conversion to PySSLError-derived objects work correctly across all modified code paths

Possibly related PRs

PySSLCertificate #6219: Introduces cert::PySSLCertificate and modifies getpeercert to return Certificate objects; overlaps with this PR's refactoring of get_peercert certificate extraction outside locks
Fix SSL deferred error #6371: Adjusts lock/handshake/deferred-certificate handling in ssl.rs; related to reducing lock hold time and deferring certificate error checks
ssl module for windows #6332: Modifies certificate/CRL loading logic including load_verify_locations and load_default_certs paths; directly related to CRL and certificate loading changes in this PR

Suggested reviewers

ShaharNaveh
coolreader18
arihant2math

Poem

🐰 Locks held too long cause threads to wait,
So parse before you seal the gate!
CRLs dance freely, certs unbound,
Less contention all around! 🔐✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'minimize ssl lock' directly and specifically summarizes the main objective of the changeset: reducing lock contention in SSL handling by restructuring argument parsing and TLS operations to minimize the duration locks are held.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot reviewed

Dec 9, 2025

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)

crates/stdlib/src/ssl.rs (1)
2594-2599: The expect() is safe but could use a more precise comment.

The is_empty() check at line 2591-2593 guarantees peer_certs is non-empty, making the expect() unreachable. However, the chain .map().last().expect() could be simplified for clarity.

Consider using last() directly on the slice:
-                peer_certs
-                    .iter()
-                    .map(|c| c.as_ref().to_vec())
-                    .last()
-                    .expect("is_empty checked above")
+                // peer_certs is guaranteed non-empty from check at line 2591
+                peer_certs.last().unwrap().as_ref().to_vec()

📜 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 db95946 and d514dc7.

📒 Files selected for processing (1)

crates/stdlib/src/ssl.rs (7 hunks)

🧰 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/ssl.rs

🧠 Learnings (1)

📓 Common learnings

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations

⏰ 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). (9)

GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Check the WASM package and demo
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Check Rust code with clippy
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Ensure compilation on various targets
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run rust tests (macos-latest)

🔇 Additional comments (4)

crates/stdlib/src/ssl.rs (4)

1360-1431: Effective lock contention reduction.

The refactoring correctly moves argument parsing and I/O operations (file reading, CRL parsing) outside the critical section. The pattern of:

Parse all arguments first

Perform CRL detection and loading separately

Acquire write locks only for the final mutations

This is a sound approach that should significantly reduce lock hold time during certificate loading operations.

2717-2720: Good pattern to avoid nested lock contention.

Cloning the Arc before acquiring the inner mutex lock prevents holding the outer RwLock while waiting for the ParkingMutex. This is a correct defensive pattern against potential deadlocks.

3548-3562: LGTM - Lock scope minimized correctly.

The pattern of extracting DER bytes within a brief lock scope, then performing all parsing and dict conversion outside the lock is correct. The early returns for None cases are properly handled within the block expression.

3586-3620: Consistent lock minimization pattern.

Both cipher() and version() methods follow the same clean pattern: acquire lock briefly to extract the cipher suite, then release and process outside the lock. This is efficient since SupportedCipherSuite is a simple copyable type.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          minimize ssl lock

0f5ffbd

youknowone force-pushed the ssl branch from d514dc7 to 0f5ffbd Compare

December 9, 2025 13:22

youknowone merged commit 5365805 into RustPython:main

Dec 9, 2025

13 checks passed

Uh oh!

There was an error while loading. Please reload this page.

youknowone deleted the ssl branch

December 9, 2025 14:01

coderabbitai Bot mentioned this pull request

Dec 18, 2025

Fix openssl build and shared ssl/error.rs #6456

Merged

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

1 participant

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

youknowone commented Dec 9, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai Bot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Dec 9, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Dec 9, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.