◐ Shell
reader mode source ↗
Skip to content

better ssl write handling#6763

Merged
youknowone merged 2 commits into
RustPython:mainfrom
youknowone:ssl-write
Jan 18, 2026
Merged

better ssl write handling#6763
youknowone merged 2 commits into
RustPython:mainfrom
youknowone:ssl-write

Conversation

@youknowone

@youknowone youknowone commented Jan 18, 2026

Copy link
Copy Markdown
Member

.

Summary by CodeRabbit

  • Bug Fixes

    • More accurate SSL/TLS error mapping (WantRead/WantWrite/Timeout/EOF) with consistent non-blocking behavior; improved detection of connection-closed vs clean close.
    • Fixed thread lifecycle signaling and counting for more reliable thread management.
  • Improvements

    • Ensure pending TLS data is flushed/drained in correct order during shutdown/EOF; drain buffered plaintext before signaling EOF.
    • Added timeout-aware I/O waits and refined non-blocking flow semantics; simplified handshake/write/read paths.
  • New Features

    • New syscall-style SSL error helper and utilities to classify blocking/closed socket conditions.
    • New socket-level tracking for buffered writable bytes and an explicit IO wait with timeout.

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

@coderabbitai

coderabbitai Bot commented Jan 18, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough
🚥 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 'better ssl write handling' directly aligns with the main changes in the changeset, which focus on SSL write path improvements, error handling, and buffering mechanics.
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

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.

@github-actions

Copy link
Copy Markdown
Contributor

Code has been automatically formatted

The code in this PR has been formatted using:

  • cargo fmt --all
    Please pull the latest changes before pushing again:
git pull origin ssl-write

@youknowone youknowone force-pushed the ssl-write branch 2 times, most recently from f602c12 to ff4e0bf Compare January 18, 2026 07:29
@youknowone youknowone marked this pull request as ready for review January 18, 2026 08:50

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

Hide comment

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@crates/stdlib/src/ssl/compat.rs`:
- Around line 1687-1788: ssl_write currently always calls writer.write_all(data)
which appends plaintext to rustls on every retry, causing duplicate application
data; add a per-socket flag on PySSLSocket (e.g., plaintext_buffered_for_write)
that ssl_write sets the first time it writes the given data (after calling
writer.write_all) and skips subsequent writer.write_all calls when that flag is
true, still proceeding to flush_pending_tls_output/send_all_bytes and handle
WANT_READ/WANT_WRITE; clear the flag once conn.wants_write() is false and after
the final flush_pending_tls_output completes (so future ssl_write calls will
write new plaintext again); update logic in ssl_write to consult this flag
(respecting is_bio mode) and ensure the flag is cleared on successful completion
or on errors that indicate the buffered plaintext was consumed or should be
discarded.

@youknowone youknowone changed the title better ssl write handlingssl_write Jan 18, 2026

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

Hide comment

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@crates/stdlib/src/ssl/compat.rs`:
- Around line 1787-1812: The timeout arm in the match on send_all_bytes
currently clears the buffered write state leading to duplicate plaintext
re-buffering on retry; modify the SslError::Timeout branch to behave like
WantWrite/WantRead by returning Err(SslError::Timeout) without clearing
socket.write_buffered_len (so pending_tls_output remains intact) and preserve
the existing semantics for is_bio vs socket-mode handling; locate the match in
the write path around send_all_bytes and update the Timeout case to not reset
write_buffered_len so retries won’t duplicate sends.
- Around line 1738-1759: Add a hash guard to detect retries with different
content: add a write_buffered_hash: PyMutex<Option<u64>> field to PySSLSocket
and initialize it to None in both PySSLSocket constructors; when you buffer new
plaintext in the block that currently sets *socket.write_buffered_len.lock() =
data.len(), compute a stable u64 hash of data and store it in
socket.write_buffered_hash.lock(); update the retry check to require both the
length and the stored hash to match (i.e., treat it as a bad write retry if
already_buffered != data.len() || buffered_hash != Some(data_hash)); and when
you clear the buffer state (where you set *socket.write_buffered_len.lock() = 0)
also clear socket.write_buffered_hash to None.
🧹 Nitpick comments (1)
crates/vm/src/stdlib/thread.rs (1)

428-454: Panic safety: run_thread lacks scopeguard protection unlike start_joinable_thread.

If a Rust panic occurs (not a Python exception), the cleanup code at lines 443-453 including fetch_sub(1) won't execute, leaving thread_count permanently elevated. The start_joinable_thread spawned thread correctly uses scopeguard::defer! for this.

Consider wrapping the cleanup in a scopeguard::defer! for consistency:

♻️ Suggested refactor
 fn run_thread(func: ArgCallable, args: FuncArgs, vm: &VirtualMachine) {
     // Increment thread count when thread actually starts executing
     vm.state.thread_count.fetch_add(1);

+    scopeguard::defer! {
+        for lock in SENTINELS.take() {
+            if lock.mu.is_locked() {
+                unsafe { lock.mu.unlock() };
+            }
+        }
+        // Clean up thread-local storage while VM context is still active
+        cleanup_thread_local_data();
+        // Clean up frame tracking
+        crate::vm::thread::cleanup_current_thread_frames(vm);
+        vm.state.thread_count.fetch_sub(1);
+    }
+
     match func.invoke(args, vm) {
         Ok(_obj) => {}
         Err(e) if e.fast_isinstance(vm.ctx.exceptions.system_exit) => {}
         Err(exc) => {
             vm.run_unraisable(
                 exc,
                 Some("Exception ignored in thread started by".to_owned()),
                 func.into(),
             );
         }
     }
-    for lock in SENTINELS.take() {
-        if lock.mu.is_locked() {
-            unsafe { lock.mu.unlock() };
-        }
-    }
-    // Clean up thread-local storage while VM context is still active
-    cleanup_thread_local_data();
-    // Clean up frame tracking
-    crate::vm::thread::cleanup_current_thread_frames(vm);
-    vm.state.thread_count.fetch_sub(1);
 }

Hide details View details @youknowone youknowone merged commit 6f26665 into RustPython:main Jan 18, 2026
13 checks passed
@youknowone youknowone deleted the ssl-write branch January 18, 2026 12:10
@coderabbitai coderabbitai Bot mentioned this pull request Jan 19, 2026
@coderabbitai coderabbitai Bot mentioned this pull request Jan 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant