-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
http2: Support trailer fields #564
Conversation
This commit adds trailer support in HTTP/2. In HTTP/1.1, chunked encoding must be used to send trialer fields. HTTP/2 deprecated any trandfer-encoding, including chunked. But trailer fields are now always available. Since trailer fields are relatively rare these days (gRPC uses them extensively though), allocating buffer for trailer fields is done when we detect that HEADERS frame containing trailer fields is started. We use Curl_add_buffer_* functions to buffer all trailers, just like we do for regular header fields. And then deliver them when stream is closed. We have to be careful here so that all data are delivered to upper layer before sending trailers to the application. We can deliver trailer field one by one using NGHTTP2_ERR_PAUSE mechanism, but current method is far more simple. Another possibility is use chunked encoding internally for HTTP/2 traffic. I have not tested it, but it could add another overhead.
By analyzing the blame information on this pull request, we identified @Andersbakken, @bagder and @dfandrich to be potential reviewers |
I tested this feature with curl and my small program using libcurl, and they worked well. |
I think the approach seems fine and is quite readable and understandable. Any changes planned or should I go ahead and merge? |
Thank you. My patch is ready to merge! |
Thanks! |
Check that the trailer buffer exists before attempting a client write for trailers on stream close. Refer to comments in curl#564
I had some segfaults with http2 that I bisected to this (15cb03a), caused by a null pointer dereference that happens when Let's take two HTTP2 websites accessed via libcurl sans the fix, https://http2.golang.org/ and https://http2.akamai.com/demo. In the case of https://http2.golang.org/ I'm also unclear about the code in onheaders to add the trailers. You're assuming that if So prior to the fix I don't see a way in either the golang or akamai case that trailers could be sent to the client, basically: I've fixed for the latter but I don't have enough experience with this module to know whether there is a bigger problem here. Also akamai isn't producing HTTP2 trailers (seems nobody is) and I don't have a way to test for actual HTTP2 trailer scenarios. |
This is really an issue. With content-length, libcurl only reads the body size left, and therefore there is no call to http2_recv, just you described. The relevant part of code is here: https://github.com/bagder/curl/blob/89a1eb7b1c5d5c49159970490375beb7385f03c1/lib/transfer.c#L422 We handles the similar situation at https://github.com/bagder/curl/blob/89a1eb7b1c5d5c49159970490375beb7385f03c1/lib/transfer.c#L311 In HTTP/2, it seems to be OK to pass maximum buffer size, since HTTP/2 layer only returns its body. diff --git a/lib/transfer.c b/lib/transfer.c
index 91777d6..0c9fb88 100644
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -410,7 +410,9 @@ static CURLcode readwrite_data(struct SessionHandle *data,
data->set.buffer_size : BUFSIZE;
size_t bytestoread = buffersize;
- if(k->size != -1 && !k->header) {
+ if(k->size != -1 && !k->header &&
+ !((conn->handler->protocol & PROTO_FAMILY_HTTP) &&
+ conn->httpversion == 20)) {
/* make sure we don't read "too much" if we can help it since we
might be pipelining and then someone else might want to read what
follows! */
You are right, on_header code is broken. Sorry about that.
nghttpd has --trailer option to specify trailer fields. nghttpd returns content-length if the requested resource (file) exists, on the other hand, if it does not exist, it returns 404 without content-length. |
PR #591 will fix bug in on_header. |
Previously, when HTTP/2 is enabled and used, and stream has content length known, Curl_read was not called when there was no bytes left to read. Because of this, we could not make sure that http2_handle_stream_close was called for every stream. Since we use http2_handle_stream_close to emit trailer fields, they were effectively ignored. This commit changes the code so that Curl_read is called even if no bytes left to read, to ensure that http2_handle_stream_close is called for every stream. Discussed in curl#564
Previously, when HTTP/2 is enabled and used, and stream has content length known, Curl_read was not called when there was no bytes left to read. Because of this, we could not make sure that http2_handle_stream_close was called for every stream. Since we use http2_handle_stream_close to emit trailer fields, they were effectively ignored. This commit changes the code so that Curl_read is called even if no bytes left to read, to ensure that http2_handle_stream_close is called for every stream. Discussed in #564
@bagder yes, there are no tests for this so I wanted to test it out manually. I did that just now with the
Here's with content length:
Here's without content length:
They both have a body and in both cases the debug build verbose output shows
libcurl output using HEADERFUNCTION is the same. |
Not sending |
I'm sorry to say I still don't get this. I applied the For example
So is this the bug you were talking about, that the trailer isn't added to the header in the case of 404? Can someone disambiguate this for me. The trailers are sent as headers, and with the headers are then sent to the header function? Because that's the way it looks like to me as I'm walking through this. I don't understand why in a header function I get 'trailer:foo' because shouldn't I get 'foo:bar' since that's my trailer? Also shouldn't I see that in the verbose output in curl if I don't have debug mode enabled? Has anyone else tested this? Edit: It appears it's allowed that 'trailer:foo' can appear in the headers to signal there is a trailer named foo. However the actual trailer 'foo:bar' isn't sent to my header function. |
For the record, 'trailer: foo' is a trailer header field, and as you understand it, it is not trailer fields (see https://tools.ietf.org/html/rfc7230#section-4.4) So, the bug in nghttpd is that 404 status does not have trailer header field ('trailer: foo'), and it is not fixed yet. With and without --no-content-length option, I can see curl emits "foo: bar" trailer fields. Here is the output of my master branch curl: Without --no-content-length:
With --no-content-length
You can see "foo:bar" appears at the end of the output in both cases. |
Sorry for not paying enough attention, but what's the status of this PR? |
I must have forgot, I will review what I have open soon. The status as of 1.6.0 was I didn't get the
|
Tried with nghttp2 v1.7.1 and latest curl master.
Without --no-content-length:
|
The reason it wasn't sent to my header function was because the test program I wrote to dump the trailers was linked to libcurl 7.46 when I compiled it. There's no longer an issue here, thanks for checking. |
Guys, I'm on
Running nghttpd like such:
I get the following response:
No trailer. |
This commit adds trailer support in HTTP/2. In HTTP/1.1, chunked
encoding must be used to send trialer fields. HTTP/2 deprecated any
trandfer-encoding, including chunked. But trailer fields are now
always available.
Since trailer fields are relatively rare these days (gRPC uses them
extensively though), allocating buffer for trailer fields is done when
we detect that HEADERS frame containing trailer fields is started. We
use Curl_add_buffer_* functions to buffer all trailers, just like we
do for regular header fields. And then deliver them when stream is
closed. We have to be careful here so that all data are delivered to
upper layer before sending trailers to the application.
We can deliver trailer field one by one using NGHTTP2_ERR_PAUSE
mechanism, but current method is far more simple.
Another possibility is use chunked encoding internally for HTTP/2
traffic. I have not tested it, but it could add another overhead.