-
-
Notifications
You must be signed in to change notification settings - Fork 706
Fix HTTP/2 requests failing with duplicate Content-Length #391
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.
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
Conversation
When Yaak moved compression, redirect, and multipart handling out of reqwest, it started explicitly adding Content-Length as an HTTP header. Over HTTP/2, hyper also sets content-length internally via DATA frame sizing, causing a duplicate that HTTP/2 servers (like Node.js) reject with PROTOCOL_ERROR. Instead of setting Content-Length as an explicit header, carry the content length through SendableBody::Stream and use http_body::Body's size_hint() to let hyper handle it automatically for both HTTP/1.1 and HTTP/2. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes HTTP/2 request failures caused by sending duplicate Content-Length headers, by stopping build_body from explicitly adding Content-Length and instead propagating known stream lengths so the HTTP client stack (hyper/reqwest) can set Content-Length appropriately.
Changes:
- Refactored
SendableBody::Streamto carry an optionalcontent_lengthalongside the stream data. - Removed explicit
Content-Lengthheader injection inbuild_body, updating tests accordingly. - Added a
SizedBodywrapper intended to exposesize_hint()for streamed bodies with known length.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/yaak-http/src/types.rs | Carries stream content length via SendableBody and removes explicit Content-Length header insertion logic/tests. |
| crates/yaak-http/src/sender.rs | Attempts to pass content-length to hyper via a custom http_body::Body wrapper for streamed requests. |
| crates/yaak-http/Cargo.toml | Adds http-body dependency to implement the custom body wrapper. |
| crates-tauri/yaak-app/src/http_request.rs | Updates pattern matching / reconstruction of SendableBody::Stream to preserve content_length. |
| Cargo.lock | Records the new http-body dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Wrap inner stream in std::sync::Mutex instead of unsafe impl Sync - Use poll_next_unpin instead of Pin::new + poll_next - Fix doc comment about Content-Length not being an HTTP/2 pseudo-header Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…oop#391) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
PROTOCOL_ERRORwhen connecting to HTTP/2-only servers (e.g., Vite dev server with HTTPS)Content-Lengthas an HTTP header, but hyper also sets it internally for HTTP/2 DATA frames, causing a duplicate that HTTP/2 servers rejectContent-Lengthas an explicit header, the content length is now carried throughSendableBody::Stream { content_length }and reported viahttp_body::Body::size_hint(), letting hyper handle it correctly for both HTTP/1.1 and HTTP/2