Fix `ssl-vendor` (OpenSSL) by joshuamegnauth54 · Pull Request #7985 · RustPython/RustPython

joshuamegnauth54 · 2026-05-26T18:46:11Z

Closes: #7893

Fix 1:
foreign-types-shared needs to match openssl's version. Bumping it is a SemVer violation because the latest versions of the crate aren't backwards compatible with older versions.
See: rust-openssl/rust-openssl#2461

Fix 2:
The second fix is to align the openssl module with the latest host_env and ssl changes.

Summary by CodeRabbit

Chores
- Updated OpenSSL-related workspace dependency with synchronization guidance
Refactor
- Enhanced OpenSSL socket handling integration for improved system compatibility

coderabbitai · 2026-05-26T18:46:26Z

📝 Walkthrough

Walkthrough

This PR refactors OpenSSL socket waiting to be VirtualMachine-aware and synchronizes a build dependency. The SocketStream::select method signature is updated to accept a &VirtualMachine parameter, the implementation switches to use sock_wait with SockWaitKind for VM-aware blocking, and all SSL operation retry paths (shutdown, handshake, write, read) thread the VM through these calls. The foreign-types-shared workspace dependency is pinned to 0.1 with a note that bumps should stay synchronized with openssl.

Changes

SSL VM-aware socket waiting

Layer / File(s)	Summary
Dependency version alignment `Cargo.toml`	`foreign-types-shared` is updated from 0.3.1 to 0.1 with a comment indicating this version should be bumped only when openssl crate bumps it.
VM-aware socket waiting implementation and integration `crates/stdlib/src/openssl.rs`	Method `SocketStream::select` and `socket_needs` are updated to accept `&VirtualMachine`, and the select implementation switches to call `sock_wait` with `SockWaitKind` derived from `SslNeeds`. All SSL operation call sites (shutdown, handshake, write, read) are updated to pass the VM when invoking these methods for VM-aware blocking behavior.

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

ShaharNaveh
youknowone

Poem

🐰 A rabbit's twist of VM delight:
Threads pass through, sockets wait just right,
Dependencies in sync so bright,
SSL reads with V-M might,
No lock-ups haunting through the night!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%.	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 'Fix ssl-vendor (OpenSSL)' is directly related to the main objective of fixing compilation failures with the ssl-vendor feature, accurately summarizing the primary change.
Linked Issues check	✅ Passed	The PR addresses the two fixes outlined in `#7893`: updating foreign-types-shared version for compatibility and aligning the openssl module with recent host_env/ssl changes to resolve all 11 compilation errors.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing ssl-vendor compilation: updating foreign-types-shared version and updating the openssl.rs module to align with recent changes.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

Closes: RustPython#7893 Fix 1: `foreign-types-shared` needs to match `openssl`'s version. Bumping it is a SemVer violation because the latest versions of the crate aren't backwards compatible with older versions. See: rust-openssl/rust-openssl#2461 Fix 2: The second fix is to align the `openssl` module with the latest `host_env` and `ssl` changes.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/stdlib/src/openssl.rs`:
- Around line 2319-2322: The match currently swallows errors from sock_wait;
change the code to propagate sock_wait errors instead of treating them as
SelectRet::Ok—call sock_wait(&*sock, wait_kind, timeout, vm)? (or otherwise map
Err to return the PyErr) and then match on the resulting bool to return
SelectRet::TimedOut or SelectRet::Ok so that errors (signals, EBADF, I/O) are
not discarded; update the block around sock_wait to use the ? operator or
explicit Err->return Err(...) propagation so exceptions bubble up instead of
being converted to SelectRet::Ok.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: c62f895b-4f7a-4004-93a8-8bfbec72063a

📥 Commits

Reviewing files that changed from the base of the PR and between dcb273b and d0b84cd.

⛔ Files ignored due to path filters (1)

Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (2)

Cargo.toml
crates/stdlib/src/openssl.rs

coderabbitai · 2026-05-26T18:52:32Z

+            match sock_wait(&*sock, wait_kind, timeout, vm) {
                Ok(true) => SelectRet::TimedOut,
                _ => SelectRet::Ok,
            }


⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Errors from sock_wait are silently discarded.

sock_wait returns PyResult<bool> and can error with EBADF, signal interrupts, or I/O failures. The wildcard _ catches both Ok(false) (correct) and Err(...) (incorrect), converting errors to SelectRet::Ok. This causes SSL operations to proceed on error and breaks signal responsiveness (e.g., Ctrl+C during handshake).

Consider propagating errors properly:

Proposed fix

- match sock_wait(&*sock, wait_kind, timeout, vm) { - Ok(true) => SelectRet::TimedOut, - _ => SelectRet::Ok, - } + match sock_wait(&*sock, wait_kind, timeout, vm) { + Ok(true) => SelectRet::TimedOut, + Ok(false) => SelectRet::Ok, + Err(_) => SelectRet::Closed, // Treat errors as socket unavailable + }

Note: This is a minimal fix. A more thorough solution would change select to return Result<SelectRet, PyBaseExceptionRef> and propagate the actual exception to callers.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

match sock_wait(&*sock, wait_kind, timeout, vm) {

Ok(true) => SelectRet::TimedOut,

_ => SelectRet::Ok,

}

match sock_wait(&*sock, wait_kind, timeout, vm) {

Ok(true) => SelectRet::TimedOut,

Ok(false) => SelectRet::Ok,

Err(_) => SelectRet::Closed,

}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stdlib/src/openssl.rs` around lines 2319 - 2322, The match currently swallows errors from sock_wait; change the code to propagate sock_wait errors instead of treating them as SelectRet::Ok—call sock_wait(&*sock, wait_kind, timeout, vm)? (or otherwise map Err to return the PyErr) and then match on the resulting bool to return SelectRet::TimedOut or SelectRet::Ok so that errors (signals, EBADF, I/O) are not discarded; update the block around sock_wait to use the ? operator or explicit Err->return Err(...) propagation so exceptions bubble up instead of being converted to SelectRet::Ok.

ShaharNaveh

TYSM for doing this!

can you please add a somewhere in the CI something that checks it?

I think adding a cargo check --features ... under

RustPython/.github/workflows/ci.yaml

Lines 139 to 141 in dcb273b

    
           - name: Test openssl build 
        
             run: cargo build --no-default-features --features ssl-openssl 
        
             if: runner.os == 'Linux'

would be fine

joshuamegnauth54 force-pushed the fix-ssl-vendor branch from f9de717 to d0b84cd Compare May 26, 2026 18:47

coderabbitai Bot reviewed May 26, 2026

View reviewed changes

ShaharNaveh requested changes May 26, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `ssl-vendor` (OpenSSL)#7985

Fix `ssl-vendor` (OpenSSL)#7985
joshuamegnauth54 wants to merge 1 commit into
RustPython:mainfrom
joshuamegnauth54:fix-ssl-vendor

joshuamegnauth54 commented May 26, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 26, 2026 •

edited

Loading

Walkthrough

Changes

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot May 26, 2026

Uh oh!

ShaharNaveh left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	- name: Test openssl build
	run: cargo build --no-default-features --features ssl-openssl
	if: runner.os == 'Linux'

Conversation

joshuamegnauth54 commented May 26, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot May 26, 2026

Choose a reason for hiding this comment

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joshuamegnauth54 commented May 26, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 26, 2026 •

edited

Loading