Forward request trailers (h2) #3241
Conversation
i110
force-pushed
the
i110/request-trailers
branch
from
2c53b77
to
fe1f3b3
Compare
There was a problem hiding this comment.
Thank you for the PR. This is looking good.
- Is it better not to merge trailers to headers even if it's non-streaming mode?
Yes.
- Any better ideas than adding a trailers argument to write_req callback like this PR currently does?
I think we are already on the right track but please see my comment below.
- Now we can't use client->pool to copy the trailers because it's actually req->pool and bound to req's lifetime
IIUC, this is a problem when response is closed before the trailers are sent by httpclient. If that happens, in on_body_on_close function of lib/core/proxy.c, we detach httpclient but keep it running (instead of calling cancel).
Assuming this understanding is correct, I think there are two ways to proceed:
- i) In http2client, retain copy of trailers so that they can be sent at a later moment.
- ii) Rename the
cancelcallback toclose, and call it when the end of the response is observed. Rationale for renaming the callback is that from the viewpoint of the user of httpclient everything is going normal. All the request up to trailers have been passed to httpclient and entire response has been received. Therefore, we are doing a normal shutdown which has to be signaled explicitly.
That said, I think my preference goes to i, considering that having a mandatory close callback is cumbersome. Note that the requirement to retain the copy of trailers is specific to http2client. http1client and http3client can serialize trailers in conjunction with the last chunk being received.
include/h2o/httpclient.h
Outdated
| @@ -292,7 +292,7 @@ struct st_h2o_httpclient_t { | |||
| * Function for writing request body. `proceed_req_cb` supplied through the `on_connect` callback will be called when the | |||
| * given data is sent to the server. Regarding the usage, refer to the doc-comment of `h2o_write_req_cb`. | |||
| */ | |||
| int (*write_req)(h2o_httpclient_t *client, h2o_iovec_t chunk, int is_end_stream); | |||
| int (*write_req)(h2o_httpclient_t *client, h2o_iovec_t chunk, h2o_headers_t *trailers, int is_end_stream); | |||
There was a problem hiding this comment.
Considering that trailers are to be provided only when is_end_stream is set, would it make sense to get rid of is_end_stream?
If trailers is NULL, it indicates that the chunk written is mid-stream. If trailers is non-NULL, that indicates that the last chunk is being provided. This trailers can be an empty list (and that would typically be the case).
|
@kazuho I pushed some commits to support forwarding trailers in non-streaming mode, along with some header utility functions (h2o_copy_header and h2o_dispose_header). Could you take a look once again please? |
lib/common/http2client.c
Outdated
| if (stream->state.req == STREAM_STATE_BODY) { | ||
| h2o_buffer_init(&stream->output.buf, &h2o_socket_buffer_prototype); | ||
| h2o_buffer_append(&stream->output.buf, body.base, body.len); | ||
| } | ||
| h2o_linklist_insert(&conn->output.sending_streams, &stream->output.sending_link); | ||
|
|
||
| /* trailers */ | ||
| if (trailers != NULL) { |
There was a problem hiding this comment.
| if (trailers != NULL) { | |
| if (trailers != NULL && num_trailers != 0) { | |
| assert(stream->output.proceed_req == NULL); |
In do_write_req, we make sure that stream->output.trailers.entries becomes non-NULL only when the list is going to be non-empty. I do not how we use trailers, but it should be consistent at least. Addition of num_trailers != 0 as well as the change to the doc-comment builds on top of the requirements that we have in do_write_req.
Separately, am I correct to understand that the user is forbidden from providing trailers if it has opted in to use request streaming? If so, it might make sense to have an assert.
There was a problem hiding this comment.
That makes sense, addressed in c61257b
Separately, am I correct to understand that the user is forbidden from providing trailers if it has opted in to use request streaming? If so, it might make sense to have an assert.
Thank you, this is a great point. Ideally we might be able to merge the passed trailers with coming request trailers, but this assertion should be sufficient for now. Addressed in 2fb2e36
| @@ -100,6 +100,7 @@ typedef h2o_httpclient_body_cb (*h2o_httpclient_head_cb)(h2o_httpclient_t *clien | |||
| typedef h2o_httpclient_head_cb (*h2o_httpclient_connect_cb)(h2o_httpclient_t *client, const char *errstr, h2o_iovec_t *method, | |||
| h2o_url_t *url, const h2o_header_t **headers, size_t *num_headers, | |||
| h2o_iovec_t *body, h2o_httpclient_proceed_req_cb *proceed_req_cb, | |||
| const h2o_header_t **trailers, size_t *num_trailers, | |||
There was a problem hiding this comment.
The way we pass the trailers seem to be diffeent between here and write_req. Is it intentional?
Unless there is a reason, it would make sense to use the same way, and based on our convention, that would be passing a list and the size; i.e.,
h2o_header_t *(as each element of the field list ish2o_header_t, the passed argument should be a pointer rather than a double pointer), andsize_tdenoting the size of the list
Co-authored-by: Kazuho Oku <kazuhooku@gmail.com>
…/request-trailers
|
@kazuho It's been quite a while, but could you please give this a review? Since you last reviewed I've addressed issues and fixed the broken test, now CI passes! |
If h2o receives trailing headers from the client after request body streaming is started, it's just discarded now. This PR let h2o forward those trailers after the last DATA frame.
Considerations:
trailersargument to write_req callback like this PR currently does?