http2: Support trailer fields #564

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@tatsuhiro-t
Contributor

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.

@tatsuhiro-t tatsuhiro-t http2: Support trailer fields
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.
f7bd526
@mention-bot

By analyzing the blame information on this pull request, we identified @Andersbakken, @bagder and @dfandrich to be potential reviewers

@tatsuhiro-t
Contributor

I tested this feature with curl and my small program using libcurl, and they worked well.
I'd like to hear that my design decision is OK...

@bagder
Member
bagder commented Dec 15, 2015

I think the approach seems fine and is quite readable and understandable. Any changes planned or should I go ahead and merge?

@bagder bagder self-assigned this Dec 15, 2015
@bagder bagder added the HTTP/2 label Dec 15, 2015
@tatsuhiro-t
Contributor

Thank you. My patch is ready to merge!

@bagder bagder added a commit that closed this pull request Dec 15, 2015
@tatsuhiro-t @bagder tatsuhiro-t + bagder http2: Support trailer fields
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.

Closes #564
15cb03a
@bagder bagder closed this in 15cb03a Dec 15, 2015
@bagder
Member
bagder commented Dec 15, 2015

Thanks!

@jay jay reopened this Jan 7, 2016
@jay jay added a commit to jay/curl that referenced this pull request Jan 7, 2016
@jay jay http2: Fix client write for trailers on stream close
Check that the trailer buffer exists before attempting a client write
for trailers on stream close.

Refer to comments in curl#564
973ee6b
@jay
Member
jay commented Jan 7, 2016

I had some segfaults with http2 that I bisected to this (15cb03a), caused by a null pointer dereference that happens when trailer_recvbuf was not created before stream close. I've pushed a fix 973ee6b that checks that the trailer buffer exists however I get the impression there may be a design issue here.

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/ http2_recv isn't called when the stream is closed (presumably because there's no more bytes to receive), therefore there's no call to http2_handle_stream_close. In the case of https://http2.akamai.com/demo it appears that the content length is unknown (I'm deducing that from the missing header), so http2_recv is called when the stream is closed and it then calls http2_handle_stream_close. Should there be such a difference?

I'm also unclear about the code in onheaders to add the trailers. You're assuming that if stream->bodystarted then trailer_recvbufpoints somewhere. I don't see how you can make that assumption unless it holds true that bodystarted means the frame type will always == NGHTTP2_HEADERS and that on_begin_headers will have definitely already been called with frame type NGHTTP2_HEADERS and bodystarted == true (which is when trailer_recvbuf is initialized).

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:
golang: there's no http2_handle_stream_close therefore no CLIENTWRITE_HEADER trailer
akamai: there's http2_handle_stream_close but null deref because no trailer_recvbuf

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.

@tatsuhiro-t
Contributor

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/ http2_recv isn't called when the stream is closed (presumably because there's no more bytes to receive), therefore there's no call to http2_handle_stream_close. In the case of https://http2.akamai.com/demo it appears that the content length is unknown (I'm deducing that from the missing header), so http2_recv is called when the stream is closed and it then calls http2_handle_stream_close. Should there be such a difference?

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.
So something like this could fix this:

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! */

I'm also unclear about the code in onheaders to add the trailers. You're assuming that if stream->bodystarted then trailer_recvbufpoints somewhere. I don't see how you can make that assumption unless it holds true that bodystarted means the frame type will always == NGHTTP2_HEADERS and that on_begin_headers will have definitely already been called with frame type NGHTTP2_HEADERS and bodystarted == true (which is when trailer_recvbuf is initialized).

You are right, on_header code is broken. Sorry about that.
The block started at https://github.com/bagder/curl/blob/973ee6b/lib/http2.c#L774 should be moved to L820. Since frame type passed to on_header is either PUSH_PROMISE or HEADERS, and if bodystarted, it must be trailer fields (libnghttp2 guarantees this).
I'll make a PR for this particular fix.

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.

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.
This behaviour is not future proof, but you can test both server scenarios.

@tatsuhiro-t
Contributor

PR #591 will fix bug in on_header.
The issue that final http2_recv is not called with content-length is not included here.

@jay jay added a commit that referenced this pull request Jan 8, 2016
@tatsuhiro-t @jay tatsuhiro-t + jay http2: Fix PUSH_PROMISE headers being treated as trailers
Discussed in #564
984d1e9
@tatsuhiro-t tatsuhiro-t added a commit to tatsuhiro-t/curl that referenced this pull request Jan 8, 2016
@tatsuhiro-t tatsuhiro-t http2: Ensure that http2_handle_stream_close is called
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
cf11823
@tatsuhiro-t
Contributor

Thank you for merging #591.
#595 solves the left over. This is the same fix I proposed in the above comment.

@jay jay added a commit that referenced this pull request Jan 8, 2016
@tatsuhiro-t @jay tatsuhiro-t + jay http2: Ensure that http2_handle_stream_close is called
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
b019af4
@bagder
Member
bagder commented Jan 11, 2016

After #591 and #595 have been merged, is this PR still valid?

@jay
Member
jay commented Jan 11, 2016

@bagder yes, there are no tests for this so I wanted to test it out manually. I did that just now with the --trailer nghttpd but in the case of no content length 404 the trailer is omitted from the header. In the case of content length the trailer looks wrong. It looks to always start with 'trailer:'. Again I'm not familiar with this area so there may not be an issue anymore, but I'm just trying to understand what is supposed to happen here.

nghttpd 4433 server.key server.crt --trailer 'foo: bar'

Here's with content length:

< HTTP/2.0 200
< server:nghttpd nghttp2/1.6.0
< content-length:196
< cache-control:max-age=3600
< date:Mon, 11 Jan 2016 23:48:29 GMT
< last-modified:Mon, 11 Jan 2016 23:21:43 GMT
< content-type:text/plain
< trailer:foo

Here's without content length:

< HTTP/2.0 404
< server:nghttpd nghttp2/1.6.0
< date:Mon, 11 Jan 2016 23:48:50 GMT
< content-type:text/html; charset=UTF-8

They both have a body and in both cases the debug build verbose output shows * h2 trailer: foo: bar. Tested using 5d7c937 2016-01-08,

curl 7.47.0-DEV (i386-pc-win32) libcurl/7.47.0-DEV OpenSSL/1.0.2e nghttp2/1.6.0
Protocols: dict file ftp ftps gopher http https imap imaps ldap pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS Debug Largefile NTLM SSL HTTP2

libcurl output using HEADERFUNCTION is the same.

@tatsuhiro-t
Contributor

Not sending trailer: foo is a bug of nghttpd. If you see trailer field "foo: bar" is emitted in curl, then it is working.
FYI, if you use latest nghttp2 master, nghttpd has --no-content-length option that strips content-length header field from response.

@jay
Member
jay commented Jan 17, 2016

I'm sorry to say I still don't get this. I applied the --no-content-length patch to nghttp2 1.6.0, thanks for that. Now I am able to produce an HTTP/2 200 with no content length, and I can see it contains 'trailer:foo' in the headers. HTTP/2 404 does not have that in the headers though.

For example nghttpd 4433 server.key server.crt --no-content-length --trailer 'foo: bar'

< HTTP/2.0 200
< server:nghttpd nghttp2/1.6.0
< cache-control:max-age=3600
< date:Sun, 17 Jan 2016 23:22:15 GMT
< last-modified:Sun, 17 Jan 2016 22:55:45 GMT
< content-type:text/plain
< trailer:foo
< HTTP/2.0 404
< server:nghttpd nghttp2/1.6.0
< date:Sun, 17 Jan 2016 23:22:37 GMT
< content-type:text/html; charset=UTF-8

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.

@tatsuhiro-t
Contributor

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:

$ curl -D- -k https://localhost:3000/ -o /dev/null -s
HTTP/2.0 200
server:nghttpd nghttp2/1.6.1-DEV
cache-control:max-age=3600
date:Mon, 18 Jan 2016 03:47:44 GMT
content-length:15
last-modified:Fri, 12 Jul 2013 14:55:22 GMT
trailer:foo

foo:bar

With --no-content-length

$ curl -D- -k https://localhost:3000/ -o /dev/null -s
HTTP/2.0 200
server:nghttpd nghttp2/1.6.1-DEV
cache-control:max-age=3600
date:Mon, 18 Jan 2016 03:47:57 GMT
last-modified:Fri, 12 Jul 2013 14:55:22 GMT
trailer:foo

foo:bar

You can see "foo:bar" appears at the end of the output in both cases.
Of course, if I omitted -D option, all header and trailer fields were not displayed, so I assumed that it was working.

@bagder
Member
bagder commented Feb 14, 2016

Sorry for not paying enough attention, but what's the status of this PR?

@jay
Member
jay commented Feb 15, 2016

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 foo:bar trailer in my header callback. I'll try with 1.7.1. Can you try with 1.7.1 also and see if you can reproduce? It makes me uneasy we don't have automated tests for http2 things like this.

nghttpd 4433 server.key server.crt --no-content-length --trailer 'foo: bar'
@tatsuhiro-t
Contributor

Tried with nghttp2 v1.7.1 and latest curl master.
With --no-content-length:

$ src/curl -D- -k https://localhost:4433/ -o /dev/null -s
HTTP/2.0 200
server:nghttpd nghttp2/1.7.1
cache-control:max-age=3600
date:Mon, 15 Feb 2016 12:33:37 GMT
last-modified:Tue, 30 Sep 2014 12:40:52 GMT
content-type:text/html
trailer:foo

foo:bar

Without --no-content-length:

$ src/curl -D- -k https://localhost:4433/ -o /dev/null -s
HTTP/2.0 200
server:nghttpd nghttp2/1.7.1
cache-control:max-age=3600
date:Mon, 15 Feb 2016 12:33:51 GMT
content-length:10
last-modified:Tue, 30 Sep 2014 12:40:52 GMT
content-type:text/html
trailer:foo

foo:bar
@jay
Member
jay commented Feb 16, 2016

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.

@jay jay closed this Feb 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment