Handle response body fallback when no length headers present#304
Handle response body fallback when no length headers present#304ryota0624 wants to merge 6 commits intomoonbitlang:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1cd9303a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/http/parser.mbt
Outdated
| if self.body is Empty && | ||
| headers.get("content-length") is None && | ||
| headers.get("transfer-encoding") is None { | ||
| self.body = PassThrough // Read until connection close |
There was a problem hiding this comment.
This fallback sets self.body = PassThrough for every response missing Content-Length and Transfer-Encoding, but responses with status 1xx, 204, and 304 are defined to have no message body at all. In those cases (especially on persistent connections), skip_body()/read_all() will wait for connection close that may never happen, blocking subsequent requests. Please gate this branch on response code so bodyless statuses keep Empty.
Useful? React with 👍 / 👎.
Pull Request Test Coverage Report for Build 51Details
💛 - Coveralls |
Add should_read_body_until_close helper to clarify logic for determining when to read the response body until connection close. Add tests for 1xx, 204, and 304 responses to ensure no body is read.
|
Using PassThrough to wait for a connection to be closed seemed like a bad idea, so I will fix it. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83af87d58f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| current_body is Empty && | ||
| headers.get("content-length") is None && | ||
| headers.get("transfer-encoding") is None && | ||
| code >= 200 && |
There was a problem hiding this comment.
This predicate classifies all 2xx responses without Content-Length/Transfer-Encoding as body-until-close, which is incorrect for successful CONNECT (there is no HTTP message body; the stream becomes a tunnel). In this repo, Client::connect calls end_request() and then skip_response_body() before entering passthrough; when read_response sets WaitConnectionClose, skip_response_body waits for EOF and can hang against compliant proxies that keep the tunnel open.
Useful? React with 👍 / 👎.
| code != 204 && | ||
| code != 304 |
There was a problem hiding this comment.
205 Reset Content is a no-body response status, but this condition only excludes 204 and 304, so a header-only 205 response is treated as close-delimited. On keep-alive connections without length headers, callers like read_all()/skip_body() will wait for connection close instead of finishing immediately, blocking subsequent requests.
Useful? React with 👍 / 👎.
ryota0624
force-pushed
the
fix-http-body-length-not-presented
branch
from
226811c to
3c2ee97
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c2ee972c8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if request_method is Some(Connect) && code >= 200 && code < 300 { | ||
| return false |
There was a problem hiding this comment.
Client::end_request now passes the originating request method into read_response, but should_read_body_until_close only exempts successful CONNECT and still treats HEAD responses without Content-Length/Transfer-Encoding as WaitConnectionClose. For a keep-alive server that correctly returns a header-only HEAD response, skip_body()/read_all() will block waiting for connection close even though HEAD responses are always bodyless.
Useful? React with 👍 / 👎.
| n | ||
| } | ||
| PassThrough => self.transport._direct_read(buf, offset~, max_len~) | ||
| WaitConnectionClose => self.transport._direct_read(buf, offset~, max_len~) |
There was a problem hiding this comment.
The new WaitConnectionClose branch reads via self.transport._direct_read, but _get_internal_buffer is not updated to expose the transport buffer for this state. Because headers are parsed with self.transport.read_until, body bytes can already be buffered in the transport reader; bypassing that buffer drops/truncates close-delimited response bodies before EOF is reached.
Useful? React with 👍 / 👎.
Fix two issues identified in PR review: - Exclude 205 Reset Content status code from close-delimited body reading (RFC 7231 requires no payload) - Track request method to handle successful CONNECT (2xx) responses, which switch to tunnel mode with no HTTP message body Add optional request_method parameter to read_response() and track last_request_method in Client to properly handle CONNECT responses on keep-alive connections. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ryota0624
force-pushed
the
fix-http-body-length-not-presented
branch
from
3c2ee97 to
a1627f7
Compare
2 participants
This pull request enhances the HTTP response parsing logic to better comply with the HTTP/1.1 specification regarding message body length determination. It updates the parser to handle responses without explicit body length headers and adds a corresponding test to ensure correct behavior.
HTTP Response Parsing Improvements:
Reader::read_responseinsrc/http/parser.mbtto set the body kind toPassThrough(read until connection close) when neitherContent-LengthnorTransfer-Encodingheaders are present, following RFC 7230 Section 3.3.3.https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.3:~:text=7.%20%20Otherwise%2C%20this%20is%20a%20response%20message%20without%20a%20declared%20message
Testing Enhancements:
"read_response passthrough fallback (no length headers)"insrc/http/parser_wbtest.mbtto verify that the parser correctly reads the body until connection close when no length headers are provided.