Skip to content

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Jan 23, 2026

Summary by CodeRabbit

  • New Features

    • Socket address resolution now accepts bytes as well as strings for host and port.
  • Bug Fixes

    • Normalized exception unwinding across nested finally blocks to use the outer-most applicable handler.
    • Improved SSL close/read/write behavior, including Zero Return handling and partial-write semantics.
    • Fixed sendfile behavior on macOS to handle partial writes without raising errors.
    • Frame accessors for generators, coroutines, and async generators now return None when closed.
  • Chores

    • Context-copy API updated to accept a VM/context parameter.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 23, 2026

📝 Walkthrough

Walkthrough

Refactors finally/unwind resolution to prefer outer handlers, updates context copy to accept a VM and deep-clone internal hamt state, extends socket getaddrinfo to accept bytes/text for host/port, adds SSL partial-write and ZeroReturn semantics, and makes generator/coroutine frame getters return Option.

Changes

Cohort / File(s) Summary
Exception handling / codegen
crates/codegen/src/compile.rs
Rework unwind_fblock resolution to locate an outer fblock handler (indices 0..fblock_idx) and use its fb_stack_depth+1 and fb_preserve_lasti; avoid reusing inner FinallyTry handlers during unwinds.
Context management
crates/stdlib/src/contextvars.rs
Change PyContext::copy to fn copy(&self, vm: &VirtualMachine) -> Self; deep-clone underlying hamt data and wrap via into_ref(vm.ctx).
Socket / getaddrinfo
crates/stdlib/src/socket.rs
Add ArgStrOrBytesLike for host/port fields in GAIOptions; branch on Str vs Buf for host and port, perform IDNA or UTF-8 handling, and pass encoded port string when present.
SSL API & compat
crates/stdlib/src/ssl.rs, crates/stdlib/src/ssl/compat.rs
Return SSLZeroReturnError on zero-return/EOF; implement partial-write semantics in ssl_write with bytes-tracking, BIO vs non-BIO differentiated errors (WantWrite vs Syscall), and adjust read close_notify handling.
VM frame accessors
crates/vm/src/builtins/asyncgenerator.rs, crates/vm/src/builtins/coroutine.rs, crates/vm/src/builtins/generator.rs
Change ag_frame, cr_frame, and gi_frame to return Option<FrameRef> (None when closed, Some(frame) otherwise).
posix sendfile (platform nuance)
crates/vm/src/stdlib/posix.rs
On macOS, if sendfile errored but some bytes were written, return the bytes written instead of raising; raise only when nothing was written.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

Suggested reviewers

  • ShaharNaveh
  • fanninpm
  • coolreader18

Poem

🐰 I hopped through handlers, outer-first and bright,

Cloned my contexts under VM's soft light,
Bytes and strings greeted the DNS hill,
TLS writes now nibble — not gulping their fill,
Frames tuck in softly, Optional at night.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The PR title is too vague and generic. It mentions 'asyncio related' issues but the changeset addresses multiple unrelated systems: exception handling, context variables, socket/getaddrinfo, SSL, frame getters, and posix sendfile. Revise the title to be more specific about the primary changes, such as 'Update frame getters to return Option and fix exception/SSL handling' or split into multiple focused PRs.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
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.

@youknowone youknowone marked this pull request as ready for review January 23, 2026 08:21
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/stdlib/src/ssl/compat.rs (1)

1744-1782: Fix partial-write retry bug: return partial success instead of deferring to bad-write-retry check.

rustls 0.23's Writer::write() can return partial writes (accepted bytes < data.len()) when internal buffer limits are exceeded. Currently, if such a partial write occurs (line 1760) and later send_all_bytes returns WantWrite, WantRead, or Timeout (lines 1810–1828), the partial count is preserved in write_buffered_len (line 1781). On retry with the original full buffer, the condition at line 1837 (already_buffered != data.len()) incorrectly triggers a "bad write retry" error, violating OpenSSL's partial-write retry semantics.

Instead, when a partial write is followed by a WANT_* or Timeout error path, return the partial success immediately rather than attempting to re-buffer on retry:

🛠️ Suggested approach: surface partial writes on WANT_* paths

Track whether only a prefix was accepted (wrote_partial), and on WantWrite/WantRead/Timeout, return the partial count instead of deferring the retry:

         let mut bytes_written_to_rustls = 0usize;
+        let mut wrote_partial = false;
 
         if already_buffered == 0 {
             // ... write plaintext to rustls ...
             match writer.write(data) { ... }
+            wrote_partial = bytes_written_to_rustls < data.len();
             *socket.write_buffered_len.lock() = bytes_written_to_rustls;
         } else if already_buffered != data.len() {
             // bad write retry check
         }
         
         loop {
             // ... send TLS records ...
             match send_all_bytes(...) {
                 Err(SslError::WantWrite) => {
+                    if wrote_partial {
+                        *socket.write_buffered_len.lock() = 0;
+                        return Ok(bytes_written_to_rustls);
+                    }
                     return Err(SslError::WantWrite);
                 }
                 Err(SslError::WantRead) => {
+                    if wrote_partial {
+                        *socket.write_buffered_len.lock() = 0;
+                        return Ok(bytes_written_to_rustls);
+                    }
                     // ... existing logic ...
                 }
                 Err(e @ SslError::Timeout(_)) => {
+                    if wrote_partial {
+                        *socket.write_buffered_len.lock() = 0;
+                        return Ok(bytes_written_to_rustls);
+                    }
                     return Err(e);
                 }
             }
         }

(Also applies to: 1857–1871)

🤖 Fix all issues with AI agents
In `@crates/codegen/src/compile.rs`:
- Around line 1531-1554: The current loop unconditionally skips all
FBlockType::FinallyTry blocks when searching for a handler (in the block that
computes (handler, stack_depth, preserve_lasti) from self.code_stack.last()),
which can omit outer finally handlers during return unwinding; change the
condition so you only skip the finally block that is the one currently being
unwound (the block that was just removed/initiated the return) rather than all
FinallyTry blocks — i.e., remove the blanket matches!(fblock.fb_type,
FBlockType::FinallyTry) continue and instead detect the unwound block (for
example by comparing fblock.fb_stack_depth or another identifier against the
unwinding context) and skip only that specific fblock while allowing outer
FinallyTry fblocks with fb_handler to be selected via the existing
fblock.fb_handler, fblock.fb_stack_depth + 1, and fblock.fb_preserve_lasti
logic.
🧹 Nitpick comments (2)
crates/stdlib/src/socket.rs (2)

2788-2792: Add tests for bytes-like host/port inputs.
The public API now accepts bytes-like objects; coverage for bytes host/port (including invalid UTF‑8) would lock in the intended behavior.


2815-2833: Clarify the UTF‑8 requirement for byte hosts in the inline comment.
The implementation validates UTF‑8 and errors otherwise, so “bytes used as-is” can read as accepting arbitrary bytes.

Proposed comment tweak
-        // Encode host: str uses IDNA encoding, bytes used as-is
+        // Encode host: str uses IDNA encoding; bytes must be valid UTF-8 and bypass IDNA

@youknowone youknowone merged commit efce325 into RustPython:main Jan 23, 2026
14 checks passed
@youknowone youknowone deleted the fix branch January 23, 2026 10:59
This was referenced Feb 1, 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