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: add compat trailers, adjust multi-headers #15193

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@apapirovski
Member

apapirovski commented Sep 5, 2017

Some changes to compatibility layer to bring it closer to the h1 implementation.

  • Adds Http2ServerRequest trailers & rawTrailers functionality. Previously didn't work at all and the request would also never end. Which actually makes me wonder, is this even correct or am I compensating for a deeper issue?

  • Fixes behaviour of multi-headers to conform with the spec (all values but set-cookie and cookie should be comma delimited, cookie should be semi-colon delimited and only set-cookie should be an array). I think it's important we conform to the spec and also be backwards compatible, even if arrays are easier to work with for multi-values. (H2 doesn't specify this directly but it links to https://tools.ietf.org/html/rfc7230#section-3.2.2)

  • Adds setter for statusMessage that warns (same as the getter), for backwards compatibility. See expressjs/express#3390 (comment)

@jasnell, @mcollina I would also like to start a discussion re: rawHeaders if possible. The current behaviour doesn't result in true rawHeaders as we have no way of knowing in what format something like cookie was sent to us based on the headers object (could've already been joined or could've been separate). Should we just emit the raw headers object alongside headers & flags from onSessionHeaders (and then pass to onServerStream too)? If we were to add it as the last argument then it wouldn't really interfere with anything but I'm open to alternate suggestions. (We could also hide it on the headers object as non-enumerable property but that seems really dirty.)

Thanks in advance for any feedback!

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http2, test

http2: add compat trailers, adjust multi-headers
Adds Http2ServerRequest trailers & rawTrailers functionality. Also fixes
behaviour of multi-headers to conform with the spec (all values but
set-cookie and cookie should be comma delimited, cookie should be
semi-colon delimited and only set-cookie should be an array). Adds
setter for statusMessage that warns, for backwards compatibility.

Refs: expressjs/express#3390 (comment)
@mcollina

Good work, thanks for taking this up! I have left some comments, let me know what you think

if (trailers === undefined)
return [];
const tuples = Object.entries(trailers);
const flattened = Array.prototype.concat.apply([], tuples);

This comment has been minimized.

@mcollina

mcollina Sep 5, 2017

Member

Can you use [].concat(tuples) instead? Why is this needed?

This comment has been minimized.

@apapirovski

apapirovski Sep 5, 2017

Member

It's a [[name, value], [name, value]] type of structure. [].concat(tuples) won't flatten it.

That said, I would ideally like to land this without this code and instead with a real solution for rawHeaders and rawTrailers (one that actually provides the real raw data to the user). I'm going to create a new commit with that behaviour and would appreciate if you or @jasnell could review.

This comment has been minimized.

@mcollina

mcollina Sep 5, 2017

Member

I think

[].concat(...Object.entries(obj))

would read better.

@@ -164,7 +172,16 @@ class Http2ServerRequest extends Readable {
}
get trailers() {
return this[kTrailers];
return this[kTrailers] || [];

This comment has been minimized.

@mcollina

mcollina Sep 5, 2017

Member

this is not the exact behavior. The returned this[kTrailers]  will be editable, while [] will not.

This comment has been minimized.

@apapirovski

apapirovski Sep 5, 2017

Member

Hmm, good point, thanks! Will update.

);
statusMessageWarned = true;
}
}

This comment has been minimized.

@mcollina

mcollina Sep 5, 2017

Member

this same block of code is shown in two other spots, can you refactor it to a single location? It needs a unit test as well.

This comment has been minimized.

@apapirovski

apapirovski Sep 5, 2017

Member

Definitely. Will update.

@apapirovski

This comment has been minimized.

Member

apapirovski commented Sep 5, 2017

@mcollina Incorporated your feedback and also added proper handling of raw headers & trailers, instead of the dummy version we had. Also have tests for everything now. Let me know if you want me to change anything about how the raw stuff works. Thanks!

@mcollina

LGTM

@mcollina

This comment has been minimized.

@apapirovski

This comment has been minimized.

Member

apapirovski commented Sep 5, 2017

Ok, so as mentioned above, there really is an issue with streams not being ended properly. I'm going to add a commit shortly that fixes it and revises how trailers are handled slightly.

@apapirovski

This comment has been minimized.

Member

apapirovski commented Sep 5, 2017

@mcollina new commit added to handle properly ending readable streams when there are trailers or the request has no body.

@mcollina

This comment has been minimized.

Member

mcollina commented Sep 5, 2017

@jasnell

jasnell approved these changes Sep 5, 2017

@mcollina

This comment has been minimized.

Member

mcollina commented Sep 7, 2017

Landed as 6416464.

@mcollina mcollina closed this Sep 7, 2017

mcollina added a commit that referenced this pull request Sep 7, 2017

http2: add compat trailers, adjust multi-headers
Adds Http2ServerRequest trailers & rawTrailers functionality. Also fixes
behaviour of multi-headers to conform with the spec (all values but
set-cookie and cookie should be comma delimited, cookie should be
semi-colon delimited and only set-cookie should be an array). Adds
setter for statusMessage that warns, for backwards compatibility.
End readable side of the stream on trailers or bodyless requests

Refs: expressjs/express#3390 (comment)
PR-URL: #15193
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@apapirovski apapirovski deleted the apapirovski:patch-http2-compat-additions branch Sep 7, 2017

MylesBorins added a commit that referenced this pull request Sep 10, 2017

http2: add compat trailers, adjust multi-headers
Adds Http2ServerRequest trailers & rawTrailers functionality. Also fixes
behaviour of multi-headers to conform with the spec (all values but
set-cookie and cookie should be comma delimited, cookie should be
semi-colon delimited and only set-cookie should be an array). Adds
setter for statusMessage that warns, for backwards compatibility.
End readable side of the stream on trailers or bodyless requests

Refs: expressjs/express#3390 (comment)
PR-URL: #15193
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 11, 2017

http2: add compat trailers, adjust multi-headers
Adds Http2ServerRequest trailers & rawTrailers functionality. Also fixes
behaviour of multi-headers to conform with the spec (all values but
set-cookie and cookie should be comma delimited, cookie should be
semi-colon delimited and only set-cookie should be an array). Adds
setter for statusMessage that warns, for backwards compatibility.
End readable side of the stream on trailers or bodyless requests

Refs: expressjs/express#3390 (comment)
PR-URL: #15193
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 12, 2017

http2: add compat trailers, adjust multi-headers
Adds Http2ServerRequest trailers & rawTrailers functionality. Also fixes
behaviour of multi-headers to conform with the spec (all values but
set-cookie and cookie should be comma delimited, cookie should be
semi-colon delimited and only set-cookie should be an array). Adds
setter for statusMessage that warns, for backwards compatibility.
End readable side of the stream on trailers or bodyless requests

Refs: expressjs/express#3390 (comment)
PR-URL: #15193
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax added a commit to addaleax/node that referenced this pull request Sep 13, 2017

http2: add compat trailers, adjust multi-headers
Adds Http2ServerRequest trailers & rawTrailers functionality. Also fixes
behaviour of multi-headers to conform with the spec (all values but
set-cookie and cookie should be comma delimited, cookie should be
semi-colon delimited and only set-cookie should be an array). Adds
setter for statusMessage that warns, for backwards compatibility.
End readable side of the stream on trailers or bodyless requests

Refs: expressjs/express#3390 (comment)
PR-URL: nodejs#15193
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment