Skip to content

flush on WantWrite #6717

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
Jan 13, 2026
Merged

flush on WantWrite #6717

youknowone merged 1 commit into from
Jan 13, 2026

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Jan 13, 2026 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • Bug Fixes
    • Improved SSL/TLS handshake reliability by properly handling blocked writes and ensuring buffered data is fully transmitted to the server.
    • Enhanced data delivery guarantees by explicitly flushing all pending TLS records after handshake completion.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 13, 2026 •
edited
Loading

📝 Walkthrough

Walkthrough

The SSL handshake logic in compat.rs is enhanced to more thoroughly drain the rustls write buffer when WantWrite occurs during socket mode, and to explicitly flush all pending TLS data after handshake completion in non-BIO mode.

Changes

Cohort / File(s) Change Summary
SSL Handshake Buffer Flushing
crates/stdlib/src/ssl/compat.rs		 Modified handle_handshake_complete to loop and drain entire rustls write buffer on WantWrite instead of breaking early; added explicit flush of pending TLS data after handshake completion in non-BIO mode to ensure delivery to peer

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Poem

🐰 The buffers flow like morning dew,
No data left behind in queue,
Loop and drain with gentle care,
Flush it all, both here and there! 💫

🚥 Pre-merge checks | ✅ 3
✅ 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 'flush on WantWrite' is directly related to the main change, which involves flushing the rustls write buffer when a WantWrite error occurs during the SSL handshake.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches
  • 📝 Generate docstrings
📜 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 e91bc11 and 0d559d0.

📒 Files selected for processing (1)
  • crates/stdlib/src/ssl/compat.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 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/compat.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). (3)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (windows-2025)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
🔇 Additional comments (1)
crates/stdlib/src/ssl/compat.rs (1)

1240-1268: The WantWrite handling logic is correct and properly implements buffer draining with timeout protection.

The approach of catching WantWrite, performing a blocking flush, and continuing the loop ensures all TLS handshake data reaches the peer. The blocking_flush_all_pending implementation provides:

  • Timeout handling: Respects socket timeout via sock_select and returns timeout errors
  • Error resilience: Retries on transient blocking IO errors, returns on real errors
  • Bounded execution: Loops only while pending data exists, avoiding infinite loops
  • Edge case handling: Handles sent == 0 case where socket wasn't ready despite select

The data flow is correct:

  1. send_all_bytes buffers unsent data in pending_tls_output on WantWrite
  2. blocking_flush_all_pending blocks until that buffer is emptied (with timeout safeguard)
  3. Loop continues to drain remaining rustls buffer
  4. Final flush after loop handles any edge cases

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 marked this pull request as ready for review January 13, 2026 12:57
@youknowone youknowone merged commit bb57724 into RustPython:main Jan 13, 2026
13 checks passed
@youknowone youknowone deleted the ssl-wantwrite branch January 13, 2026 13:05
terryluan12 pushed a commit to terryluan12/RustPython that referenced this pull request Jan 15, 2026
@youknowone @terryluan12
flush on WantWrite (RustPython#6717)
aed9acf
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.

1 participant

@youknowone