http2: refactor how trailers are done #19959

jasnell · 2018-04-11T23:17:28Z

Rather than an option, introduce a method and an event...

server.on('stream', (stream) => {
  stream.respond(undefined, { hasTrailers: true });
  stream.on('trailers-ready', () => {
    stream.trailers({ abc: 'xyz'});
  });
  stream.end('hello world');
});

The 'trailers-ready' event is necessary because of
respondWithFile and respondWithFD... neither of
which cause the Http2Stream to emit the normal stream
related events (by design).

If using the Stream API, however, we can also do:

server.on('stream', (stream) => {
  stream.respond(undefined, { hasTrailers: true });
  stream.on('finish', () => {
    stream.trailers({ 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.

/cc @mcollina @addaleax @sebdeckers @apapirovski @trivikr @nodejs/http2

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

hiroppy · 2018-04-11T23:30:47Z

doc/api/http2.md

-if the `getTrailers` callback attempts to set such header fields.
-
-### Class: Http2Server
+s### Class: Http2Server


heh... silly typo

hiroppy

Thanks!

vsemozhetbyt

Just some questions, otherwise the docs LGTM.

vsemozhetbyt · 2018-04-11T23:33:52Z

doc/api/http2.md

@@ -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


Do we update YAML metadata when we add an options field? If yes, there are 4 such additions in this PR.

I would rather call this waitForTrailers.

vsemozhetbyt · 2018-04-11T23:40:23Z

doc/api/http2.md

+
+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


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.

vsemozhetbyt · 2018-04-11T23:45:09Z

doc/api/http2.md

@@ -1036,6 +1042,35 @@ Provides miscellaneous information about the current state of the

 A current state of this `Http2Stream`.

+#### http2stream.trailers(headers)


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)

maybe this can be sendTrailers()?

apapirovski · 2018-04-12T06:42:38Z

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 finish event rather than introducing another custom event?

mcollina

I think you should also add a test and update trailer support in the compat layer.

mcollina · 2018-04-12T08:01:14Z

lib/internal/http2/core.js

+    if (this.destroyed || this.closed)
+      throw new ERR_HTTP2_INVALID_STREAM();
+    if (this[kSentTrailers])
+      throw new ERR_HTTP2_TRAILERS_ALREADY_SENT();


Can you please add a test for this case?

mcollina · 2018-04-12T08:05:26Z

doc/api/http2.md

+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.


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.

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.

All of that should definitely be documented.

mcollina · 2018-04-12T08:07:14Z

lib/internal/http2/core.js

@@ -1745,6 +1728,31 @@ class Http2Stream extends Duplex {
    priorityFn();
  }

+  trailers(headers) {


what will happen if trailers(headers) is called before end()? This case should be tested.

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.

This is very dangerous. I would have trailers throw if the event has not be emitted yet instead.

mcollina · 2018-04-12T08:09:10Z

doc/api/http2.md

@@ -874,6 +870,16 @@ stream.on('trailers', (headers, flags) => {
 });
 ```

+#### Event: 'trailers-ready'


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()

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.

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.

+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.

mcollina · 2018-04-12T08:09:44Z

doc/api/http2.md

@@ -1036,6 +1042,35 @@ Provides miscellaneous information about the current state of the

 A current state of this `Http2Stream`.

+#### http2stream.trailers(headers)


maybe this can be sendTrailers()?

mcollina · 2018-04-12T08:10:54Z

doc/api/http2.md

@@ -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


I would rather call this waitForTrailers.

addaleax · 2018-04-12T14:01:06Z

doc/api/http2.md

-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


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?

Or a different name entirely :-)

addaleax · 2018-04-12T14:03:49Z

lib/internal/http2/core.js

-  stream[kSentTrailers] = trailers;
-  return headersList;
+    return;
+  process.nextTick(emit, stream, 'trailers-ready');


What happens if the event listener never calls .trailers()?

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.

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?

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.)

I meant basically adding if (stream.listenerCount('trailers-ready') === 0) throw new Error(); here, right before this line – nothing fancier than that

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.

jasnell · 2018-04-13T19:02:59Z

Ok, I made several updates

I renamed the hasTrailers option to waitForTrailers as requested
I renamed the trailers() function to sendTrailers() as requested
I renamed the trailers-ready event to wantTrailers ... using finish here includes a variety of complexities with the Writable API.. see http2: respondWithFile/respondWithFD events and stream API interaction #20014
I stressed the need to call close() or sendTrailers() in the docs to properly close the Http2Stream when using wantTrailers
Fixed a number of nits
Close the Http2Stream if there is no 'wantTrailers' handler.

jasnell · 2018-04-13T23:47:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14274/

jasnell · 2018-04-14T21:15:01Z

@mcollina @addaleax @nodejs/http2 .. when you get a moment, I'd appreciate another quick review. I'd like to get this landed early next week in time for the next 10.0.0 test build.

addaleax

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.

mcollina

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>

jasnell · 2018-04-16T16:55:33Z

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. 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>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 11, 2018

vsemozhetbyt added the http2 Issues or PRs related to the http2 subsystem. label Apr 11, 2018

hiroppy suggested changes Apr 11, 2018

View reviewed changes

hiroppy approved these changes Apr 11, 2018

View reviewed changes

vsemozhetbyt reviewed Apr 11, 2018

View reviewed changes

mcollina reviewed Apr 12, 2018

View reviewed changes

addaleax reviewed Apr 12, 2018

View reviewed changes

jasnell force-pushed the http2-refactor-trailers branch from b27d2a8 to 152d211 Compare April 13, 2018 18:50

addaleax approved these changes Apr 14, 2018

View reviewed changes

jasnell force-pushed the http2-refactor-trailers branch from d062e7b to c838ba7 Compare April 16, 2018 14:59

mcollina approved these changes Apr 16, 2018

View reviewed changes

jasnell closed this Apr 16, 2018

BethGriggs mentioned this pull request Oct 30, 2018

v8.13.0 proposal #23974

Merged

beevelop mentioned this pull request Nov 20, 2018

2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250

Closed

		@@ -1036,6 +1042,35 @@ Provides miscellaneous information about the current state of the

		A current state of this `Http2Stream`.

		#### http2stream.trailers(headers)

http2: refactor how trailers are done #19959

http2: refactor how trailers are done #19959

Conversation

jasnell commented Apr 11, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiroppy left a comment

Choose a reason for hiding this comment

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Apr 12, 2018 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 13, 2018 • edited Loading

jasnell commented Apr 13, 2018

jasnell commented Apr 14, 2018

addaleax left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

jasnell commented Apr 16, 2018

apapirovski commented Apr 12, 2018 •

edited

Loading

jasnell commented Apr 13, 2018 •

edited

Loading