Skip to content

ssl: store SSL socket owners as weak references - #8423

Merged
youknowone merged 1 commit into
RustPython:mainfrom
kyokuping:codex/fix-ssl-sni-dead-owner
Aug 2, 2026
Merged

ssl: store SSL socket owners as weak references#8423
youknowone merged 1 commit into
RustPython:mainfrom
kyokuping:codex/fix-ssl-sni-dead-owner

Conversation

@kyokuping

@kyokuping kyokuping commented Jul 31, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Assisted-by: Codex:gpt-5.6-sol

Summary

  • enable test_ssl.test_sni_callback_on_dead_references
  • store _SSLSocket.owner as a weak reference
  • return an SSL error before invoking the SNI callback when the owner is no longer alive
  • break the strong owner reference cycle, which also enables test_create_connection_memory_leak and test_handshake_timeout_handler_leak

Summary by CodeRabbit

  • Bug Fixes
    • Improved SSL socket owner handling to prevent stale references.
    • SSL callbacks now report an appropriate error when the associated owner is no longer available.
    • Owner assignment validates references before completing.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 37b0ecef-9a75-4a07-a0ad-05fb4e03267d

📥 Commits

Reviewing files that changed from the base of the PR and between 977c0a4 and b3e4f6f.

⛔ Files ignored due to path filters (2)
  • Lib/test/test_asyncio/test_ssl.py is excluded by !Lib/**
  • Lib/test/test_ssl.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/stdlib/src/ssl.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/stdlib/src/ssl.rs
📝 Walkthrough

Walkthrough

SSL socket owners now use weak Python references. Socket and BIO construction propagate weak-reference conversion errors. Owner access upgrades references, and SNI callbacks return CALLBACK_FAILED when the owner has been collected.

Changes

SSL owner lifecycle

Layer / File(s) Summary
Weak owner storage and construction
crates/stdlib/src/ssl.rs		 SSL and BIO socket construction now downgrade owners to weak references. The socket owner field uses PyWeak.
Weak owner access and callback handling
crates/stdlib/src/ssl.rs		 SNI callbacks upgrade the owner and return CALLBACK_FAILED when it no longer exists. The owner getter upgrades the reference, and the setter returns weak-reference conversion errors.

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

🚥 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 and concisely describes the main change: storing SSL socket owners as weak references.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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 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.

@github-actions

github-actions Bot commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/ssl.py
[x] test: cpython/Lib/test/test_ssl.py (TODO: 10)

dependencies:

  • ssl

dependent tests: (53 tests)

  • ssl: test_asyncio test_ftplib test_httplib test_httpservers test_imaplib test_logging test_poplib test_ssl test_urllib test_urllib2_localnet test_venv test_xmlrpc
    • asyncio.selector_events: test_asyncio
    • ftplib: test_urllib2
      • urllib.request: test_http_cookiejar test_pathlib test_pydoc test_sax test_site test_urllib2net test_urllibnet
    • http.client: test_docxmlrpc test_hashlib test_ucn test_unicodedata test_wsgiref
      • logging.handlers: test_concurrent_futures test_pkgutil
    • http.server: test_robotparser
      • pydoc: test_enum
    • smtplib: test_smtplib test_smtpnet
    • urllib.request:
      • pathlib: test_ast test_dbm_sqlite3 test_ensurepip test_importlib test_json test_launcher test_pathlib test_peg_generator test_pyrepl test_runpy test_tarfile test_tempfile test_tomllib test_tools test_traceback test_unparse test_winapi test_zipapp test_zipfile test_zoneinfo test_zstd

[x] lib: cpython/Lib/asyncio
[ ] test: cpython/Lib/test/test_asyncio (TODO: 31)

dependencies:

  • asyncio

dependent tests: (7 tests)

  • asyncio: test_asyncio test_external_inspection test_inspect test_logging test_os test_pdb test_unittest

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@youknowone youknowone added the z-ca-2026 Tag to track Contribution Academy 2026 label Aug 1, 2026
@kyokuping
kyokuping force-pushed the codex/fix-ssl-sni-dead-owner branch from f7e845d to 977c0a4 Compare August 1, 2026 15:09
@kyokuping
kyokuping marked this pull request as ready for review August 1, 2026 23:31
coderabbitai[bot]
coderabbitai Bot reviewed Aug 1, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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/ssl.rs`:
- Around line 1923-1928: Use a shared owner-conversion helper in
crates/stdlib/src/ssl.rs to treat Python None as absent before creating PyWeak.
Apply it at lines 1923-1928 and 2005-2010, and update the owner assignment at
lines 3983-3986 to clear the owner when given None.
🪄 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 Plus

Run ID: b182b78d-b8a5-468f-ad72-6194f4a99ef4

📥 Commits

Reviewing files that changed from the base of the PR and between 9e92f96 and 977c0a4.

⛔ Files ignored due to path filters (2)
  • Lib/test/test_asyncio/test_ssl.py is excluded by !Lib/**
  • Lib/test/test_ssl.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/stdlib/src/ssl.rs

Comment thread crates/stdlib/src/ssl.rs
@kyokuping
ssl: store SSL socket owners as weak references
b3e4f6f 
Assisted-by: Codex:gpt-5.6-sol
@kyokuping
kyokuping force-pushed the codex/fix-ssl-sni-dead-owner branch from 977c0a4 to b3e4f6f Compare August 2, 2026 04:58
youknowone
youknowone approved these changes Aug 2, 2026
View reviewed changes
Comment thread crates/stdlib/src/ssl.rs
session_was_reused: PyMutex<bool>,
// Owner (SSLSocket instance that owns this _SSLSocket)
owner: PyRwLock<Option<PyObjectRef>>,
owner: PyRwLock<Option<PyRef<PyWeak>>>,

@youknowone youknowone Aug 2, 2026

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 12f3646 into RustPython:main Aug 2, 2026
27 of 28 checks passed
@kyokuping
kyokuping deleted the codex/fix-ssl-sni-dead-owner branch August 8, 2026 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

z-ca-2026 Tag to track Contribution Academy 2026

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kyokuping @youknowone