-
Notifications
You must be signed in to change notification settings - Fork 30k
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: refactor how trailers are done #19959
Conversation
doc/api/http2.md
Outdated
if the `getTrailers` callback attempts to set such header fields. | ||
|
||
### Class: Http2Server | ||
s### Class: Http2Server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh... silly typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some questions, otherwise the docs LGTM.
doc/api/http2.md
Outdated
@@ -694,8 +694,8 @@ added: v8.4.0 | |||
* `weight` {number} Specifies the relative dependency of a stream in relation | |||
to other streams with the same `parent`. The value is a number between `1` | |||
and `256` (inclusive). | |||
* `getTrailers` {Function} Callback function invoked to collect trailer | |||
headers. | |||
* `hasTrailers` {boolean} When `true`, the `Http2Stream` will emit the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we update YAML metadata when we add an options field? If yes, there are 4 such additions in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather call this waitForTrailers
.
doc/api/http2.md
Outdated
|
||
Sends a trailing `HEADERS` frame to the connected HTTP/2 peer. This method | ||
will cause the `Http2Stream` to be immediately closed and should only be | ||
called when the last DATA frame has been sent to the peer. When sending a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this is a subject for this PR, but there is some inconsistency in frame names (HEADERS, DATA) — sometimes they are wrapped in backticks, sometimes not.
doc/api/http2.md
Outdated
@@ -1036,6 +1042,35 @@ Provides miscellaneous information about the current state of the | |||
|
|||
A current state of this `Http2Stream`. | |||
|
|||
#### http2stream.trailers(headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we OK with using non-verbs for method names? There are some other examples in http2 API, so this is just to be sure that this is intended)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this can be sendTrailers()
?
I assume the main reason here is that this enables async behaviour while gather the trailer headers, as opposed to the old API which forced sync behaviour? Also, I've only had a chance to skim but what would be the effort level to always hang it off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should also add a test and update trailer support in the compat layer.
lib/internal/http2/core.js
Outdated
if (this.destroyed || this.closed) | ||
throw new ERR_HTTP2_INVALID_STREAM(); | ||
if (this[kSentTrailers]) | ||
throw new ERR_HTTP2_TRAILERS_ALREADY_SENT(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a test for this case?
doc/api/http2.md
Outdated
When the `options.hasTrailers` option is set, the `'trailers-ready'` event will | ||
be emitted immediately after queuing the last chunk of payload data to be sent. | ||
The `http2stream.trailers()` method can then be used to sent trailing header | ||
fields to the peer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if hasTrailers: true
, then the user must call .trailers()
to close the writable side of the stream. This is not being stressed enough in the sentence. As far as I understand, if .trailers()
is not called, there will be a memory leak.
This seems a dangerous API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They would need to call trailers()
or close()
. Alternatively, we could have it such that if there is no handler for the 'trailers-ready'
event that it would close automatically but I didn't necessarily want to tie it specifically to that. This case is really no different than what we have currently with most streams things in that the end()
must be called to close it out. This is also why hasTrailers
defaults to false
. Users who want to send trailers need to understand that there's an extra step involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of that should definitely be documented.
lib/internal/http2/core.js
Outdated
@@ -1745,6 +1728,31 @@ class Http2Stream extends Duplex { | |||
priorityFn(); | |||
} | |||
|
|||
trailers(headers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what will happen if trailers(headers)
is called before end()
? This case should be tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stream then ends before sending any additional data and any pending data still in the queue is lost. I agree that a test would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very dangerous. I would have trailers
throw if the event has not be emitted yet instead.
doc/api/http2.md
Outdated
@@ -874,6 +870,16 @@ stream.on('trailers', (headers, flags) => { | |||
}); | |||
``` | |||
|
|||
#### Event: 'trailers-ready' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that the other side of the duplex can have trailers as well, maybe we need a better name for this, or maybe just emit 'finish'
in the cases it's a respondWithFile()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could but when using respondWithFile()
, one of the things that happens automatically is that end()
is called on the stream API interface to ensure that someone does not attempt to write to the Duplex
after calling respondWithFile()
... I believe that will have the result of causing the stream to emit 'finish'
as part of the stream API processing. I'd prefer to have a single event that would work for both cases so I'm open to names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use the 'finish'
event if we change the way we handle closing the Duplex
when respondWith*
is used. Basically, don't end the Writable side but error if write()
is used after respondWith*
is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for two separate things.
I'm not necessarily certain that calling end()
is the right thing to do in that case. But that's another issue.
doc/api/http2.md
Outdated
@@ -1036,6 +1042,35 @@ Provides miscellaneous information about the current state of the | |||
|
|||
A current state of this `Http2Stream`. | |||
|
|||
#### http2stream.trailers(headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this can be sendTrailers()
?
doc/api/http2.md
Outdated
@@ -694,8 +694,8 @@ added: v8.4.0 | |||
* `weight` {number} Specifies the relative dependency of a stream in relation | |||
to other streams with the same `parent`. The value is a number between `1` | |||
and `256` (inclusive). | |||
* `getTrailers` {Function} Callback function invoked to collect trailer | |||
headers. | |||
* `hasTrailers` {boolean} When `true`, the `Http2Stream` will emit the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather call this waitForTrailers
.
doc/api/http2.md
Outdated
The HTTP/1 specification forbids trailers from containing HTTP/2 pseudo-header | ||
fields (e.g. `':method'`, `':path'`, etc). An `'error'` event will be emitted | ||
if the `getTrailers` callback attempts to set such header fields. | ||
When the `options.hasTrailers` option is set, the `'trailers-ready'` event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our docs currently list 21 event names that consist of more than one word; All of them use lowerCamelCase
, so it might be good to be consistent with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or a different name entirely :-)
lib/internal/http2/core.js
Outdated
stream[kSentTrailers] = trailers; | ||
return headersList; | ||
return; | ||
process.nextTick(emit, stream, 'trailers-ready'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the event listener never calls .trailers()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it doesn't close and things hang until it does. Exactly the same as if end()
is never called on an HTTP response
object.
The stream can be closed using either trailers()
or close()
on the Http2Stream
.
That should definitely be emphasized more in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay … I don’t see any way to do that better, I guess, but could we at least make it an early error if there are no listeners for the event at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
potentially, yes, but doing so would lock us in to having those trailers available right when that event is triggered when it could be any time after that event. Will be stewing on this more.
(this, btw, is the part I've been stewing on for a while with this which is why it took so long to get this done. I haven't been able to come up with a better solution.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant basically adding if (stream.listenerCount('trailers-ready') === 0) throw new Error();
here, right before this line – nothing fancier than that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or just simply close()
rather than throw. That's certainly possible. The trailers-ready
(or whatever we use) is a marker that says trailers can be sent at any time after this. If the user is not listening for that, then they cannot reasonably know when to proceed with sending the trailers anyway.
b27d2a8
to
152d211
Compare
Ok, I made several updates
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Rather than an option, introduce a method and an event... ```js server.on('stream', (stream) => { stream.respond(undefined, { waitForTrailers: true }); stream.on('wantTrailers', () => { stream.sendTrailers({ abc: 'xyz'}); }); stream.end('hello world'); }); ``` This is a breaking change in the API such that the prior `options.getTrailers` is no longer supported at all. Ordinarily this would be semver-major and require a deprecation but the http2 stuff is still experimental.
d062e7b
to
c838ba7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Rather than an option, introduce a method and an event... ```js server.on('stream', (stream) => { stream.respond(undefined, { waitForTrailers: true }); stream.on('wantTrailers', () => { stream.sendTrailers({ abc: 'xyz'}); }); stream.end('hello world'); }); ``` This is a breaking change in the API such that the prior `options.getTrailers` is no longer supported at all. Ordinarily this would be semver-major and require a deprecation but the http2 stuff is still experimental. PR-URL: #19959 Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Landed in 237aa7e |
Rather than an option, introduce a method and an event... ```js server.on('stream', (stream) => { stream.respond(undefined, { waitForTrailers: true }); stream.on('wantTrailers', () => { stream.sendTrailers({ abc: 'xyz'}); }); stream.end('hello world'); }); ``` This is a breaking change in the API such that the prior `options.getTrailers` is no longer supported at all. Ordinarily this would be semver-major and require a deprecation but the http2 stuff is still experimental. PR-URL: #19959 Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Rather than an option, introduce a method and an event... ```js server.on('stream', (stream) => { stream.respond(undefined, { waitForTrailers: true }); stream.on('wantTrailers', () => { stream.sendTrailers({ abc: 'xyz'}); }); stream.end('hello world'); }); ``` This is a breaking change in the API such that the prior `options.getTrailers` is no longer supported at all. Ordinarily this would be semver-major and require a deprecation but the http2 stuff is still experimental. PR-URL: nodejs#19959 Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Rather than an option, introduce a method and an event... ```js server.on('stream', (stream) => { stream.respond(undefined, { waitForTrailers: true }); stream.on('wantTrailers', () => { stream.sendTrailers({ abc: 'xyz'}); }); stream.end('hello world'); }); ``` This is a breaking change in the API such that the prior `options.getTrailers` is no longer supported at all. Ordinarily this would be semver-major and require a deprecation but the http2 stuff is still experimental. PR-URL: nodejs#19959 Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Rather than an option, introduce a method and an event... ```js server.on('stream', (stream) => { stream.respond(undefined, { waitForTrailers: true }); stream.on('wantTrailers', () => { stream.sendTrailers({ abc: 'xyz'}); }); stream.end('hello world'); }); ``` This is a breaking change in the API such that the prior `options.getTrailers` is no longer supported at all. Ordinarily this would be semver-major and require a deprecation but the http2 stuff is still experimental. PR-URL: nodejs#19959 Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Rather than an option, introduce a method and an event... ```js server.on('stream', (stream) => { stream.respond(undefined, { waitForTrailers: true }); stream.on('wantTrailers', () => { stream.sendTrailers({ abc: 'xyz'}); }); stream.end('hello world'); }); ``` This is a breaking change in the API such that the prior `options.getTrailers` is no longer supported at all. Ordinarily this would be semver-major and require a deprecation but the http2 stuff is still experimental. PR-URL: nodejs#19959 Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Rather than an option, introduce a method and an event... ```js server.on('stream', (stream) => { stream.respond(undefined, { waitForTrailers: true }); stream.on('wantTrailers', () => { stream.sendTrailers({ abc: 'xyz'}); }); stream.end('hello world'); }); ``` This is a breaking change in the API such that the prior `options.getTrailers` is no longer supported at all. Ordinarily this would be semver-major and require a deprecation but the http2 stuff is still experimental. Backport-PR-URL: #22850 PR-URL: #19959 Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Rather than an option, introduce a method and an event...
The
'trailers-ready'
event is necessary because ofrespondWithFile
andrespondWithFD
... neither ofwhich cause the
Http2Stream
to emit the normal streamrelated events (by design).
If using the Stream API, however, we can also do:
This is a breaking change in the API such that the prior
options.getTrailers
is no longer supported at all.Ordinarily this would be semver-major and require a
deprecation but the http2 stuff is still experimental.
/cc @mcollina @addaleax @sebdeckers @apapirovski @trivikr @nodejs/http2
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes