From fa5a3809a304b28246684a9a9e980184ffe03f0e Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 11 Apr 2018 16:11:35 -0700 Subject: [PATCH] http2: refactor how trailers are done 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: https://github.com/nodejs/node/pull/22850 PR-URL: https://github.com/nodejs/node/pull/19959 Reviewed-By: Yuta Hiroto Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina --- doc/api/errors.md | 13 ++ doc/api/http2.md | 192 +++++++++++------- lib/internal/errors.js | 8 +- lib/internal/http2/compat.js | 7 +- lib/internal/http2/core.js | 91 ++++----- src/node_http2.cc | 107 ++++------ src/node_http2.h | 26 +-- ...est-http2-client-request-options-errors.js | 1 - ...est-http2-compat-serverrequest-trailers.js | 19 +- ...ompat-serverresponse-createpushresponse.js | 19 +- .../test-http2-misused-pseudoheaders.js | 23 +-- .../test-http2-no-wanttrailers-listener.js | 35 ++++ test/parallel/test-http2-respond-errors.js | 37 +--- .../test-http2-respond-file-errors.js | 3 +- .../test-http2-respond-file-fd-errors.js | 3 +- test/parallel/test-http2-sent-headers.js | 7 +- test/parallel/test-http2-trailers.js | 40 +++- 17 files changed, 326 insertions(+), 305 deletions(-) create mode 100644 test/parallel/test-http2-no-wanttrailers-listener.js diff --git a/doc/api/errors.md b/doc/api/errors.md index 1c5836e553df1a..1ab394580e7b76 100755 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -861,6 +861,19 @@ When setting the priority for an HTTP/2 stream, the stream may be marked as a dependency for a parent stream. This error code is used when an attempt is made to mark a stream and dependent of itself. + +### ERR_HTTP2_TRAILERS_ALREADY_SENT + +Trailing headers have already been sent on the `Http2Stream`. + + +### ERR_HTTP2_TRAILERS_NOT_READY + +The `http2stream.sendTrailers()` method cannot be called until after the +`'wantTrailers'` event is emitted on an `Http2Stream` object. The +`'wantTrailers'` event will only be emitted if the `waitForTrailers` option +is set for the `Http2Stream`. + ### ERR_HTTP2_UNSUPPORTED_PROTOCOL diff --git a/doc/api/http2.md b/doc/api/http2.md index ea5449733b051b..47b5bb8af8148f 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -176,13 +176,13 @@ immediately following the `'frameError'` event. added: v8.4.0 --> -The `'goaway'` event is emitted when a GOAWAY frame is received. When invoked, +The `'goaway'` event is emitted when a `GOAWAY` frame is received. When invoked, the handler function will receive three arguments: -* `errorCode` {number} The HTTP/2 error code specified in the GOAWAY frame. +* `errorCode` {number} The HTTP/2 error code specified in the `GOAWAY` frame. * `lastStreamID` {number} The ID of the last stream the remote peer successfully processed (or `0` if no ID is specified). -* `opaqueData` {Buffer} If additional opaque data was included in the GOAWAY +* `opaqueData` {Buffer} If additional opaque data was included in the `GOAWAY` frame, a `Buffer` instance will be passed containing that data. *Note*: The `Http2Session` instance will be shut down automatically when the @@ -193,7 +193,7 @@ the handler function will receive three arguments: added: v8.4.0 --> -The `'localSettings'` event is emitted when an acknowledgment SETTINGS frame +The `'localSettings'` event is emitted when an acknowledgment `SETTINGS` frame has been received. When invoked, the handler function will receive a copy of the local settings. @@ -214,7 +214,7 @@ session.on('localSettings', (settings) => { added: v8.4.0 --> -The `'remoteSettings'` event is emitted when a new SETTINGS frame is received +The `'remoteSettings'` event is emitted when a new `SETTINGS` frame is received from the connected peer. When invoked, the handler function will receive a copy of the remote settings. @@ -385,7 +385,7 @@ added: v8.11.2 * `code` {number} An HTTP/2 error code * `lastStreamID` {number} The numeric ID of the last processed `Http2Stream` * `opaqueData` {Buffer|TypedArray|DataView} A `TypedArray` or `DataView` - instance containing additional data to be carried within the GOAWAY frame. + instance containing additional data to be carried within the `GOAWAY` frame. Transmits a `GOAWAY` frame to the connected peer *without* shutting down the `Http2Session`. @@ -419,7 +419,7 @@ added: v8.4.0 * Value: {boolean} Indicates whether or not the `Http2Session` is currently waiting for an -acknowledgment for a sent SETTINGS frame. Will be `true` after calling the +acknowledgment for a sent `SETTINGS` frame. Will be `true` after calling the `http2session.settings()` method. Will be `false` once all sent SETTINGS frames have been acknowledged. @@ -554,9 +554,9 @@ Once called, the `http2session.pendingSettingsAck` property will be `true` while the session is waiting for the remote peer to acknowledge the new settings. -*Note*: The new settings will not become effective until the SETTINGS +*Note*: The new settings will not become effective until the `SETTINGS` acknowledgment is received and the `'localSettings'` event is emitted. It -is possible to send multiple SETTINGS frames while acknowledgment is still +is possible to send multiple `SETTINGS` frames while acknowledgment is still pending. #### http2session.type @@ -699,8 +699,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. + * `waitForTrailers` {boolean} When `true`, the `Http2Stream` will emit the + `'wantTrailers'` event after the final `DATA` frame has been sent. * Returns: {ClientHttp2Stream} @@ -727,15 +727,15 @@ req.on('response', (headers) => { }); ``` -When set, the `options.getTrailers()` function is called immediately after -queuing the last chunk of payload data to be sent. The callback is passed a -single object (with a `null` prototype) that the listener may use to specify -the trailing header fields to send to the peer. +When the `options.waitForTrailers` option is set, the `'wantTrailers'` event +is emitted immediately after queuing the last chunk of payload data to be sent. +The `http2stream.sendTrailers()` method can then be called to send trailing +headers to the peer. -*Note*: 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. +It is important to note that when `options.waitForTrailers` is set, the +`Http2Stream` will *not* automatically close when the final `DATA` frame is +transmitted. User code *must* call either `http2stream.sendTrailers()` or +`http2stream.close()` to close the `Http2Stream`. The `:method` and `:path` pseudo-headers are not specified within `headers`, they respectively default to: @@ -881,6 +881,16 @@ stream.on('trailers', (headers, flags) => { }); ``` +#### Event: 'wantTrailers' + + +The `'wantTrailers'` event is emitted when the `Http2Stream` has queued the +final `DATA` frame to be sent on a frame and the `Http2Stream` is ready to send +trailing headers. When initiating a request or response, the `waitForTrailers` +option must be set for this event to be emitted. + #### http2stream.aborted + +* `headers` {HTTP/2 Headers Object} + +Sends a trailing `HEADERS` frame to the connected HTTP/2 peer. This method +will cause the `Http2Stream` to be immediately closed and must only be +called after the `'wantTrailers'` event has been emitted. When sending a +request or sending a response, the `options.waitForTrailers` option must be set +in order to keep the `Http2Stream` open after the final `DATA` frame so that +trailers can be sent. + +```js +const http2 = require('http2'); +const server = http2.createServer(); +server.on('stream', (stream) => { + stream.respond(undefined, { waitForTrailers: true }); + stream.on('wantTrailers', () => { + stream.sendTrailers({ xyz: 'abc' }); + }); + stream.end('Hello World'); +}); +``` + +The HTTP/1 specification forbids trailers from containing HTTP/2 pseudo-header +fields (e.g. `':method'`, `':path'`, etc). + ### Class: ClientHttp2Stream