better ssl write handling#6763
Conversation
📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin ssl-write |
Sorry, something went wrong.
f602c12 to
ff4e0bf
Compare
January 18, 2026 07:29
There was a problem hiding this 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.
Sorry, something went wrong.
There was a problem hiding this 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_threadlacksscopeguardprotection unlikestart_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, leavingthread_countpermanently elevated. Thestart_joinable_threadspawned thread correctly usesscopeguard::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); }
Sorry, something went wrong.
6f26665
into
RustPython:main
Jan 18, 2026
.
Summary by CodeRabbit
Bug Fixes
Improvements
New Features
✏️ Tip: You can customize this high-level summary in your review settings.