move chunked encoding code to http1 protocol handler #1819
Conversation
There was a problem hiding this comment.
Thank you for the PR. I like this simplification.
How about doing something like 8847f14? I think it further simplifies the design.
Also, I would suggest adding assert(req->res.content_length != SIZE_MAX) to finalostream_start_pull.
Previously, all responses that did not have the content-length set were converted to chunked encoding, because the chunked ostream filter was injected before the code path reached http1.c. That is no longer the case now with the changes in this PR, and we only use chunked encoding when push mode is used. Therefore, We need to either:
- make sure that the user of pull mode always sets the content-length
- when pull mode is invoked, check if content-length is set, and if not set, switch to chunked encoding
My understanding is that we always set content-length when using pull mode, and that the former is sufficient.
lib/http1.c
Outdated
|
|
||
| if (!self->sent_headers) { | ||
| conn->req.timestamps.response_start_at = h2o_gettimeofday(conn->super.ctx->loop); | ||
| if (conn->req.send_server_timing_header) | ||
| h2o_add_server_timing_header(&conn->req); | ||
|
|
||
| if (should_use_chunked_encoding(req)) { | ||
| self->chunked.enabled = 1; |
There was a problem hiding this comment.
Don't we need to do either of:
- set this flag to zero if
should_use_chunked_encodingreturns false - clear the flag in
init_request
Otherwise, I believe that there would be a chance of server using chunked encoding even when it should not, while responding to a succeeding HTTP request of a persistent connection.
… "encode_chunked" concentrate on building the prefix and suffixes
|
@kazuho thank you for the review. I addressed the issues. |
|
Thank you for the changes. Would it be possible for us to have an end-to-test for chunked-encoding in pull mode? My understanding is that that has been added because it will be used. Then, we need to have tests; otherwise we will not notice it becoming broken (which IMO could happen, because these encoders are generally fragile to changes). |
|
@i110 Thank you for all your work! I have brutally pushed some changes to this PR, making minor adjustments and merging master. With the changes, I think that the PR is ready for merge. Would you mind reviewing the changes I added, and click the merge button if they look OK to you? Or, please let me know if you see any issue / have concerns. |
Since chunked encoding code is implemented as a filter being applied to all requests (including h2 and subreq), it's getting harder to maintain it for various reasons: many unnecessary checks, increasing code path, etc. This PR removes chunked filter and encapsulate chunked encoding implementation in http1.c
This is just for refactor and doesn't change behavior (I believe).