http2: Support trailer fields #564

tatsuhiro-t · 2015-12-14T14:01:52Z

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.

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.

mention-bot · 2015-12-14T14:01:57Z

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

tatsuhiro-t · 2015-12-14T14:03:27Z

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 · 2015-12-15T08:19:33Z

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

tatsuhiro-t · 2015-12-15T13:16:52Z

Thank you. My patch is ready to merge!

bagder · 2015-12-15T22:48:57Z

Thanks!

Check that the trailer buffer exists before attempting a client write for trailers on stream close. Refer to comments in curl#564

jay · 2016-01-07T03:45:42Z

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 · 2016-01-07T12:32:46Z

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 · 2016-01-07T13:15:12Z

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

Discussed in #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 curl#564

tatsuhiro-t · 2016-01-08T12:36:24Z

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

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 · 2016-01-11T22:51:30Z

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

jay · 2016-01-11T23:53:16Z

@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 · 2016-01-12T12:02:16Z

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 · 2016-01-17T23:32:07Z

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 · 2016-01-18T03:53:50Z

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 · 2016-02-14T10:01:21Z

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

jay · 2016-02-15T08:15:18Z

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 · 2016-02-15T12:35:08Z

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 · 2016-02-16T02:23:02Z

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.

pcasaretto · 2017-10-12T15:17:01Z

Guys, I'm on

curl 7.56.0 (x86_64-apple-darwin16.7.0) libcurl/7.56.0 OpenSSL/1.0.2l zlib/1.2.8 nghttp2/1.26.0
Release-Date: 2017-10-04
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy

Running nghttpd like such:

λ nghttpd 4433  cert.key cert.crt --no-content-length --trailer 'foo: bar'

I get the following response:

λ curl -D- -k https://localhost:4433/ -o /dev/null -s 
HTTP/2 404
server: nghttpd nghttp2/1.26.0
date: Thu, 12 Oct 2017 15:13:42 GMT
trailer: foo
content-type: text/html; charset=UTF-8
content-length: 147

text/html; charset=UTF-8

No trailer.
What am I missing?

tatsuhiro-t mentioned this pull request Dec 14, 2015

Trailers not output for HTTP2 #561

Closed

bagder self-assigned this Dec 15, 2015

bagder added the HTTP/2 label Dec 15, 2015

bagder closed this in 15cb03a Dec 15, 2015

jay reopened this Jan 7, 2016

jay added a commit to jay/curl that referenced this pull request Jan 7, 2016

http2: Fix client write for trailers on stream close

973ee6b

Check that the trailer buffer exists before attempting a client write for trailers on stream close. Refer to comments in curl#564

tatsuhiro-t mentioned this pull request Jan 7, 2016

http2: Fix bug that PUSH_PROMISE headers could be treated as trailer … #591

Closed

jay pushed a commit that referenced this pull request Jan 8, 2016

http2: Fix PUSH_PROMISE headers being treated as trailers

984d1e9

Discussed in #564

tatsuhiro-t mentioned this pull request Jan 8, 2016

http2: Ensure that http2_handle_stream_close is called #595

Closed

jay closed this Feb 16, 2016

lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: Support trailer fields #564

http2: Support trailer fields #564

tatsuhiro-t commented Dec 14, 2015

mention-bot commented Dec 14, 2015

tatsuhiro-t commented Dec 14, 2015

bagder commented Dec 15, 2015

tatsuhiro-t commented Dec 15, 2015

bagder commented Dec 15, 2015

jay commented Jan 7, 2016

tatsuhiro-t commented Jan 7, 2016

tatsuhiro-t commented Jan 7, 2016

tatsuhiro-t commented Jan 8, 2016

bagder commented Jan 11, 2016

jay commented Jan 11, 2016

tatsuhiro-t commented Jan 12, 2016

jay commented Jan 17, 2016

tatsuhiro-t commented Jan 18, 2016

bagder commented Feb 14, 2016

jay commented Feb 15, 2016

tatsuhiro-t commented Feb 15, 2016

jay commented Feb 16, 2016

pcasaretto commented Oct 12, 2017

http2: Support trailer fields #564

http2: Support trailer fields #564

Conversation

tatsuhiro-t commented Dec 14, 2015

mention-bot commented Dec 14, 2015

tatsuhiro-t commented Dec 14, 2015

bagder commented Dec 15, 2015

tatsuhiro-t commented Dec 15, 2015

bagder commented Dec 15, 2015

jay commented Jan 7, 2016

tatsuhiro-t commented Jan 7, 2016

tatsuhiro-t commented Jan 7, 2016

tatsuhiro-t commented Jan 8, 2016

bagder commented Jan 11, 2016

jay commented Jan 11, 2016

tatsuhiro-t commented Jan 12, 2016

jay commented Jan 17, 2016

tatsuhiro-t commented Jan 18, 2016

bagder commented Feb 14, 2016

jay commented Feb 15, 2016

tatsuhiro-t commented Feb 15, 2016

jay commented Feb 16, 2016

pcasaretto commented Oct 12, 2017