fix: support bytes in StreamingResponse generators#1308
fix: support bytes in StreamingResponse generators#1308
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds byte-capable streaming end-to-end: Python StreamingResponse/SSEResponse now accept generators yielding bytes or str; Rust StreamingResponse gains a media_type field and handles bytes vs strings; three new streaming endpoints and integration tests exercise binary, file, and text streams. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Router as Robyn Router
participant Rust as Rust Streaming Handler
participant PyGen as Python Generator
Client->>Router: GET /stream/bytes
Router->>Rust: invoke endpoint -> StreamingResponse(media_type, generator)
Rust->>Client: send response headers (status, Content-Type)
Rust->>PyGen: start generator (sync/async)
loop per yield
PyGen->>Rust: yield (bytes or str)
Rust->>Rust: if str -> encode to bytes
Rust->>Client: stream bytes chunk
end
PyGen->>Rust: StopIteration / end
Rust->>Client: close stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
sansyrox
changed the title
sansyrox
force-pushed
the
fix/sse-downloads
branch
from
3e5bb99 to
8d81bad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
src/types/response.rs (1)
562-577:⚠️ Potential issue | 🟡 MinorFallback to
"text/event-stream"on extraction failure could mask bugs.If
media_typeextraction fails (lines 570, 575), the code silently falls back to"text/event-stream", which would cause SSE headers to be injected for what might be a binary streaming response. Consider returning an error instead, since a missing or non-extractablemedia_typeon an object that passed thehas_media_typecheck (line 510) would indicate a real problem.
🤖 Fix all issues with AI agents
In `@src/types/response.rs`:
- Around line 115-123: The SSE-specific headers are being added twice: once
during extraction in FromPyObject (where Headers may get "Cache-Control" and
"Connection") and again unconditionally in respond_to after
apply_hashmap_headers; update respond_to (the method that calls
apply_hashmap_headers and then appends SSE headers) to only append each SSE
header if it is not already present on self.headers (or use whatever lookup
method the Headers type provides), so avoid duplicate "Connection",
"Cache-Control", "X-Accel-Buffering", "Pragma", and "Expires" values; reference
respond_to, apply_hashmap_headers, FromPyObject, and the Headers struct to
locate and implement the conditional checks.
🧹 Nitpick comments (2)
integration_tests/base_routes.py (1)
1359-1372: RedundantContent-Typeheader alongsidemedia_type.The
Content-Typeis specified both inheadersand implicitly viamedia_type. Currently this works becausemedia_typeonly auto-sets headers for"text/event-stream", but if that logic changes, these could conflict. Consider setting onlymedia_typeand letting the framework derive theContent-Typeheader, or document why both are needed.This same pattern repeats in the
/stream/bytes_file(line 1391-1394) and/stream/mixed_text(line 1411) endpoints.robyn/responses.py (1)
124-151: Consider auto-settingContent-Typefrommedia_typefor non-SSE streaming responses.Currently,
StreamingResponse.__init__only setsContent-Typewhenmedia_type == "text/event-stream"(line 149). For other media types (e.g.,"application/octet-stream"), users must manually pass theContent-Typeheader themselves. This creates a subtle API pitfall wheremedia_typecontrols SSE header injection but does not set the response'sContent-Typefor non-SSE types.Suggested improvement
# Set default SSE headers if media_type == "text/event-stream": self.headers.set("Content-Type", "text/event-stream") # Cache-Control and Connection headers are set by Rust layer with optimized headers + else: + # Set Content-Type from media_type if not already provided + if not self.headers.contains("Content-Type"): + self.headers.set("Content-Type", media_type)
Merging this PR will not alter performance
Performance Changes
Comparing 
|
sansyrox
force-pushed
the
fix/sse-downloads
branch
from
018bf11 to
79eef64
Compare
sansyrox
force-pushed
the
main
branch
4 times, most recently
from
5fb08e0 to
0472e42
Compare
sansyrox
force-pushed
the
fix/sse-downloads
branch
from
3ca21d3 to
2af6d3f
Compare
sansyrox
force-pushed
the
fix/sse-downloads
branch
from
6c240d4 to
e69d5c4
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/types/response.rs (1)
564-580:⚠️ Potential issue | 🟡 MinorRisky default: falling back to
"text/event-stream"on extraction error.If
media_typeis present but has an unexpected type (passes thehasattrcheck on line 513 but failsextract::<String>), this silently defaults to"text/event-stream". That would inject SSE headers into a binary streaming response — contrary to the user's intent.Consider returning the extraction error instead of silently defaulting:
Proposed fix
let media_type: String = match obj.getattr("media_type") { Ok(attr) => match attr.extract() { Ok(media_type) => { debug!("Successfully extracted media_type: {}", media_type); media_type } Err(e) => { debug!("Failed to extract media_type: {}", e); - "text/event-stream".to_string() + return Err(e); } }, Err(e) => { debug!("Failed to get media_type attribute: {}", e); - "text/event-stream".to_string() + return Err(e); } };
sansyrox
force-pushed
the
fix/sse-downloads
branch
from
3e0435a to
d2ec793
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/types/response.rs (1)
151-159: Consider logging a warning for unsupported yield types.When the generator yields a value that is neither
PyBytesnorString, the stream terminates silently by returningNone. This could make debugging difficult for users who accidentally yield an unsupported type.💡 Proposed enhancement
Ok(value) => { if let Ok(py_bytes) = value.downcast::<PyBytes>() { Some((py_bytes.as_bytes().to_vec(), generator)) } else if let Ok(s) = value.extract::<String>() { Some((s.into_bytes(), generator)) } else { + log::warn!( + "Generator yielded unsupported type: {}. Expected bytes or str.", + value.get_type().name().unwrap_or("unknown") + ); None } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/response.rs` around lines 151 - 159, In the Ok(value) arm in src/types/response.rs (the branch that checks for PyBytes and String), add a warning log before returning None when the yielded value is neither PyBytes nor String; include the Python object's type name and a short repr/str in the warning so users can see what unsupported type was yielded (use the existing logging facility in the project, e.g. tracing::warn! or log::warn!, and gather type/repr from the PyAny/Python API) and then return None as before; this change should be made around the existing symbols value, PyBytes, String, and generator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/types/response.rs`:
- Around line 166-170: There are extra closing braces and an orphan `None` in
the block around the function in src/types/response.rs (look for the function or
impl that returns an Option/Result near the end of the method containing the
stray `None`); remove the superfluous `}` characters and either remove or
correctly place the `None` return so the function's control flow and indentation
match the intended match/if expression (ensuring the function's final return
matches its signature), then reformat to restore correct brace alignment for the
surrounding function/impl.
---
Nitpick comments:
In `@src/types/response.rs`:
- Around line 151-159: In the Ok(value) arm in src/types/response.rs (the branch
that checks for PyBytes and String), add a warning log before returning None
when the yielded value is neither PyBytes nor String; include the Python
object's type name and a short repr/str in the warning so users can see what
unsupported type was yielded (use the existing logging facility in the project,
e.g. tracing::warn! or log::warn!, and gather type/repr from the PyAny/Python
API) and then return None as before; this change should be made around the
existing symbols value, PyBytes, String, and generator.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99db1d81-ba76-453c-8900-622371472c94
📒 Files selected for processing (4)
integration_tests/base_routes.pyintegration_tests/test_binary_streaming.pyrobyn/responses.pysrc/types/response.rs
✅ Files skipped from review due to trivial changes (1)
- integration_tests/test_binary_streaming.py
🚧 Files skipped from review as they are similar to previous changes (1)
- integration_tests/base_routes.py
| } | ||
| } | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Malformed code structure will cause compilation failure.
Lines 166-170 contain extra closing braces and an orphan None statement with incorrect indentation. This appears to be leftover from a merge or edit error and will prevent the Rust code from compiling.
🐛 Proposed fix
}
Err(e) => {
if !e.is_instance_of::<pyo3::exceptions::PyStopIteration>(py) {
log::error!("Generator error: {}", e);
}
None
}
}
- }
- None
- }
})
})
.await📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| } | |
| None | |
| } | |
| } | |
| } | |
| }) | |
| }) | |
| .await |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/response.rs` around lines 166 - 170, There are extra closing braces
and an orphan `None` in the block around the function in src/types/response.rs
(look for the function or impl that returns an Option/Result near the end of the
method containing the stray `None`); remove the superfluous `}` characters and
either remove or correctly place the `None` return so the function's control
flow and indentation match the intended match/if expression (ensuring the
function's final return matches its signature), then reformat to restore correct
brace alignment for the surrounding function/impl.
1 participant
Description
This PR fixes #1236
Summary
This PR
PR Checklist
Please ensure that:
Pre-Commit Instructions:
Summary by CodeRabbit
New Features
Bug Fixes
Tests