From 141f166bda5b2b54c718a5cf5ecd5c996a2ee7b2 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 11 Apr 2018 13:59:26 -0700 Subject: [PATCH 01/67] http2: fix ping duration calculation PR-URL: https://github.com/nodejs/node/pull/19956 Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: Trivikram Kamat --- src/node_http2.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index e9a06a88635882..036decb89581c9 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2832,8 +2832,8 @@ void Http2Session::Http2Ping::Send(uint8_t* payload) { } void Http2Session::Http2Ping::Done(bool ack, const uint8_t* payload) { - session_->statistics_.ping_rtt = (uv_hrtime() - startTime_); - double duration = (session_->statistics_.ping_rtt - startTime_) / 1e6; + session_->statistics_.ping_rtt = uv_hrtime() - startTime_; + double duration = session_->statistics_.ping_rtt / 1e6; Local buf = Undefined(env()->isolate()); if (payload != nullptr) { From f3f4ee69240916683ec5806970b1ea92751a1ec2 Mon Sep 17 00:00:00 2001 From: Indranil Dasgupta Date: Sat, 14 Apr 2018 15:19:45 +0200 Subject: [PATCH 02/67] doc: close event does not take arguments Explicitly added in the docs that the close event does not expect any arguments when invoked. Fixes: https://github.com/nodejs/node/issues/20018 PR-URL: https://github.com/nodejs/node/pull/20031 Reviewed-By: Vse Mozhet Byt Reviewed-By: Trivikram Kamat Reviewed-By: James M Snell --- doc/api/http2.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index df70278a071732..ea5449733b051b 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -128,7 +128,8 @@ solely on the API of the `Http2Session`. added: v8.4.0 --> -The `'close'` event is emitted once the `Http2Session` has been destroyed. +The `'close'` event is emitted once the `Http2Session` has been destroyed. Its +listener does not expect any arguments. #### Event: 'connect' -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 +* `session` {Http2Session} +* `socket` {net.Socket} + The `'connect'` event is emitted once the `Http2Session` has been successfully connected to the remote peer and communication may begin. From 19e41a6d783f6ab955fd0036d3d3ec9260ea386d Mon Sep 17 00:00:00 2001 From: "Christine E. Taylor" Date: Fri, 20 Apr 2018 20:21:44 -0700 Subject: [PATCH 06/67] test: add strictEqual method to assert Adds strictEqual method to assert on stream.session.alpnProtocol to verify expected value 'h2'. This changes 'h2' from an assertion error message to the value expected from stream.session.alpnProtocol. PR-URL: https://github.com/nodejs/node/pull/20189 Reviewed-By: Rich Trott Reviewed-By: Richard Lau Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Benjamin Gruenbaum Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater --- test/parallel/test-http2-create-client-secure-session.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http2-create-client-secure-session.js b/test/parallel/test-http2-create-client-secure-session.js index 1f20ec8e42a871..8b2aa1c168cb5e 100644 --- a/test/parallel/test-http2-create-client-secure-session.js +++ b/test/parallel/test-http2-create-client-secure-session.js @@ -21,7 +21,7 @@ function onStream(stream, headers) { const socket = stream.session[kSocket]; assert(stream.session.encrypted); - assert(stream.session.alpnProtocol, 'h2'); + assert.strictEqual(stream.session.alpnProtocol, 'h2'); const originSet = stream.session.originSet; assert(Array.isArray(originSet)); assert.strictEqual(originSet[0], From ac14fecdcd8ea971badd27302cb60f352fe7615d Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Sun, 22 Apr 2018 02:49:45 +0530 Subject: [PATCH 07/67] doc: add parameters for Http2Session:error event Add parameters for the callback for the Http2Session:error event inline with the pattern in the rest of the documentation. Refs: https://github.com/nodejs/help/issues/877#issuecomment-381253464 PR-URL: https://github.com/nodejs/node/pull/20206 Reviewed-By: Matteo Collina Reviewed-By: Vse Mozhet Byt Reviewed-By: Luigi Pinca Reviewed-By: Trivikram Kamat --- doc/api/http2.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/api/http2.md b/doc/api/http2.md index a32fd36764b2a8..93c7b8dff442ac 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -149,6 +149,8 @@ connected to the remote peer and communication may begin. added: v8.4.0 --> +* `error` {Error} + The `'error'` event is emitted when an error occurs during the processing of an `Http2Session`. From 110ac587387af261a36ffd43c2a9d9e5c34ed875 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Tue, 24 Apr 2018 02:47:21 +0530 Subject: [PATCH 08/67] doc: improve docs for Http2Session:frameError Improve documentation regarding the callback parameters for the frameError event for instances of Http2Session, making it inline with the currently accepted structure, like the rest of the documentation. Refs: https://github.com/nodejs/help/issues/877#issuecomment-381253464 PR-URL: https://github.com/nodejs/node/pull/20236 Reviewed-By: Vse Mozhet Byt Reviewed-By: Trivikram Kamat Reviewed-By: Ruben Bridgewater --- doc/api/http2.md | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index 93c7b8dff442ac..7d9cbadd1de917 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -159,18 +159,16 @@ an `Http2Session`. added: v8.4.0 --> +* `type` {integer} The frame type. +* `code` {integer} The error code. +* `id` {integer} The stream id (or `0` if the frame isn't associated with a + stream). + The `'frameError'` event is emitted when an error occurs while attempting to send a frame on the session. If the frame that could not be sent is associated with a specific `Http2Stream`, an attempt to emit `'frameError'` event on the `Http2Stream` is made. -When invoked, the handler function will receive three arguments: - -* An integer identifying the frame type. -* An integer identifying the error code. -* An integer identifying the stream (or 0 if the frame is not associated with - a stream). - If the `'frameError'` event is associated with a stream, the stream will be closed and destroyed immediately following the `'frameError'` event. If the event is not associated with a stream, the `Http2Session` will be shut down From df3e6d9da468264a6081b187a8fd7fbf402bdf07 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 17 Apr 2018 11:37:50 +0200 Subject: [PATCH 09/67] http: added aborted property to request PR-URL: https://github.com/nodejs/node/pull/20094 Reviewed-By: James M Snell Reviewed-By: Luigi Pinca --- doc/api/http.md | 12 +++++++++- doc/api/http2.md | 10 ++++++++ lib/_http_client.js | 6 ++++- lib/_http_incoming.js | 2 ++ lib/_http_server.js | 1 + lib/internal/http2/compat.js | 7 ++++++ test/parallel/test-http-aborted.js | 26 +++++++++++++++++++++ test/parallel/test-http2-compat-aborted.js | 27 ++++++++++++++++++++++ 8 files changed, 89 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-http-aborted.js create mode 100644 test/parallel/test-http2-compat-aborted.js diff --git a/doc/api/http.md b/doc/api/http.md index 0640ec417d0b88..ecffb87be4bbaf 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -1401,7 +1401,7 @@ following additional events, methods, and properties. added: v0.3.8 --> -Emitted when the request has been aborted and the network socket has closed. +Emitted when the request has been aborted. ### Event: 'close' + +* {boolean} + +The `message.aborted` property will be `true` if the request has +been aborted. + ### message.destroy([error]) + +* {boolean} + +The `request.aborted` property will be `true` if the request has +been aborted. + #### request.destroy([error]) -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. * `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` frame, a `Buffer` instance will be passed containing that data. -*Note*: The `Http2Session` instance will be shut down automatically when the -`'goaway'` event is emitted. +The `'goaway'` event is emitted when a `GOAWAY` frame is received. + +The `Http2Session` instance will be shut down automatically when the `'goaway'` +event is emitted. #### Event: 'localSettings' +* `settings` {HTTP/2 Settings Object} A copy of the `SETTINGS` frame received. + 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. +has been received. *Note*: When using `http2session.settings()` to submit new settings, the modified settings do not take effect until the `'localSettings'` event is @@ -216,9 +217,10 @@ session.on('localSettings', (settings) => { added: v8.4.0 --> +* `settings` {HTTP/2 Settings Object} A copy of the `SETTINGS` frame 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. +from the connected peer. ```js session.on('remoteSettings', (settings) => { From d9efc870e39343d4a794fa59262e2292da1d5b2b Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Mon, 30 Apr 2018 14:18:53 +0200 Subject: [PATCH 16/67] http2: reduce require calls in http2/core This commit removes unnecesary requires of http and internal/util in http2/core.js PR-URL: https://github.com/nodejs/node/pull/20422 Reviewed-By: Anatoli Papirovski Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Luigi Pinca Reviewed-By: Trivikram Kamat --- lib/internal/http2/core.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 5fa8e87ddbd04b..a42e416851af32 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -23,9 +23,12 @@ const { onServerStream, Http2ServerResponse, } = require('internal/http2/compat'); const { utcDate } = require('internal/http'); -const { promisify } = require('internal/util'); +const { + promisify, + customInspectSymbol: kInspect +} = require('internal/util'); const { isArrayBufferView } = require('internal/util/types'); -const { _connectionListener: httpConnectionListener } = require('http'); +const { _connectionListener: httpConnectionListener } = http; const { createPromise, promiseResolve } = process.binding('util'); const debug = util.debuglog('http2'); @@ -67,7 +70,6 @@ const { constants, nameForErrorCode } = binding; const NETServer = net.Server; const TLSServer = tls.Server; -const kInspect = require('internal/util').customInspectSymbol; const { kIncomingMessage } = require('_http_common'); const { kServerResponse } = require('_http_server'); From ce96ad06a112ebffb4282db8b50101f415c54c51 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Mon, 30 Apr 2018 15:12:57 +0200 Subject: [PATCH 17/67] http2: rename http2_state class to Http2State This commit renames the http2_state class to follow the guidelines in CPP_STYLE_GUIDE.md. PR-URL: https://github.com/nodejs/node/pull/20423 Reviewed-By: Anna Henningsen --- src/env-inl.h | 4 ++-- src/env.h | 6 +++--- src/node_http2.cc | 2 +- src/node_http2_state.h | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index bdf3e8ae453f9f..e30370633fd3da 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -417,12 +417,12 @@ inline void Environment::set_http_parser_buffer(char* buffer) { http_parser_buffer_ = buffer; } -inline http2::http2_state* Environment::http2_state() const { +inline http2::Http2State* Environment::http2_state() const { return http2_state_.get(); } inline void Environment::set_http2_state( - std::unique_ptr buffer) { + std::unique_ptr buffer) { CHECK(!http2_state_); // Should be set only once. http2_state_ = std::move(buffer); } diff --git a/src/env.h b/src/env.h index 7b4a6999755bcf..c098ca1d2b9a9d 100644 --- a/src/env.h +++ b/src/env.h @@ -617,8 +617,8 @@ class Environment { inline char* http_parser_buffer() const; inline void set_http_parser_buffer(char* buffer); - inline http2::http2_state* http2_state() const; - inline void set_http2_state(std::unique_ptr state); + inline http2::Http2State* http2_state() const; + inline void set_http2_state(std::unique_ptr state); inline double* fs_stats_field_array() const; inline void set_fs_stats_field_array(double* fields); @@ -759,7 +759,7 @@ class Environment { double* heap_space_statistics_buffer_ = nullptr; char* http_parser_buffer_; - std::unique_ptr http2_state_; + std::unique_ptr http2_state_; double* fs_stats_field_array_; diff --git a/src/node_http2.cc b/src/node_http2.cc index 2744c8bb43df51..237e5c01b91696 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2868,7 +2868,7 @@ void Initialize(Local target, Isolate* isolate = env->isolate(); HandleScope scope(isolate); - std::unique_ptr state(new http2_state(isolate)); + std::unique_ptr state(new Http2State(isolate)); #define SET_STATE_TYPEDARRAY(name, field) \ target->Set(context, \ diff --git a/src/node_http2_state.h b/src/node_http2_state.h index ed88f068a04b16..64a0942f7ffa67 100644 --- a/src/node_http2_state.h +++ b/src/node_http2_state.h @@ -84,9 +84,9 @@ namespace http2 { IDX_SESSION_STATS_COUNT }; -class http2_state { +class Http2State { public: - explicit http2_state(v8::Isolate* isolate) : + explicit Http2State(v8::Isolate* isolate) : root_buffer( isolate, sizeof(http2_state_internal)), From 807807f7e9673369a6f3219604d932824897dfc2 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Sun, 6 May 2018 00:31:03 +0530 Subject: [PATCH 18/67] doc: add parameters for Http2Session:stream event Add parameters for the callback for the Http2Session:stream event inline with the pattern in the rest of the documentation. Refs: https://github.com/nodejs/help/issues/877#issuecomment-381253464 PR-URL: https://github.com/nodejs/node/pull/20547 Reviewed-By: Anna Henningsen Reviewed-By: Vse Mozhet Byt Reviewed-By: Matteo Collina Reviewed-By: Trivikram Kamat --- doc/api/http2.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index c2fa1de02288a7..0db1e044cac2e4 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -233,10 +233,13 @@ session.on('remoteSettings', (settings) => { added: v8.4.0 --> -The `'stream'` event is emitted when a new `Http2Stream` is created. When -invoked, the handler function will receive a reference to the `Http2Stream` -object, a [HTTP/2 Headers Object][], and numeric flags associated with the -creation of the stream. +* `stream` {Http2Stream} A reference to the stream +* `headers` {HTTP/2 Headers Object} An object describing the headers +* `flags` {number} The associated numeric flags +* `rawHeaders` {Array} An array containing the raw header names followed by + their respective values. + +The `'stream'` event is emitted when a new `Http2Stream` is created. ```js const http2 = require('http2'); From 7d23d638c6a7bb64e2a34f9a8992bfb0b2ff0852 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Sun, 6 May 2018 08:01:02 +0200 Subject: [PATCH 19/67] test: fix flaky http2-flow-control test PR-URL: https://github.com/nodejs/node/pull/20556 Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Matteo Collina --- test/parallel/test-http2-misbehaving-flow-control-paused.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/parallel/test-http2-misbehaving-flow-control-paused.js b/test/parallel/test-http2-misbehaving-flow-control-paused.js index 60a2cdabf847d9..26d2ed5dd244a2 100644 --- a/test/parallel/test-http2-misbehaving-flow-control-paused.js +++ b/test/parallel/test-http2-misbehaving-flow-control-paused.js @@ -70,8 +70,6 @@ server.on('stream', (stream) => { client.destroy(); })); stream.on('end', common.mustNotCall()); - stream.respond(); - stream.end('ok'); }); server.listen(0, () => { From e3e16d7ceabc673f33d0e4ff5de1faf00a9cd65b Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Tue, 8 May 2018 17:59:14 +0530 Subject: [PATCH 20/67] doc: add params for ClientHttp2Session:altsvc Add parameters for the callback for the ClientHttp2Session:altsvc event inline with the pattern in the rest of the documentation. Refs: https://github.com/nodejs/help/issues/877#issuecomment-381253464 PR-URL: https://github.com/nodejs/node/pull/20598 Reviewed-By: Vse Mozhet Byt Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Trivikram Kamat Reviewed-By: Colin Ihrig --- doc/api/http2.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index 0db1e044cac2e4..bb8bc8dc461a10 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -667,9 +667,9 @@ added: v8.4.0 added: v8.11.2 --> -* `alt`: {string} -* `origin`: {string} -* `streamId`: {number} +* `alt` {string} +* `origin` {string} +* `streamId` {number} The `'altsvc'` event is emitted whenever an `ALTSVC` frame is received by the client. The event is emitted with the `ALTSVC` value, origin, and stream From 776c3d009f8ef60baa50c1d5d93de0240b86fa52 Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Wed, 9 May 2018 01:16:57 +0530 Subject: [PATCH 21/67] doc: add parameters for Http2Stream:error event Add parameters for the callback for the Http2Stream:error event inline with the pattern in the rest of the documentation. Refs: https://github.com/nodejs/help/issues/877#issuecomment-381253464 PR-URL: https://github.com/nodejs/node/pull/20610 Reviewed-By: Vse Mozhet Byt Reviewed-By: Trivikram Kamat Reviewed-By: Colin Ihrig --- doc/api/http2.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/api/http2.md b/doc/api/http2.md index bb8bc8dc461a10..7c162d1062d2c8 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -850,6 +850,8 @@ code specified when closing the stream. If the code is any value other than added: v8.4.0 --> +* `error` {Error} + The `'error'` event is emitted when an error occurs during the processing of an `Http2Stream`. From 11f194ff7491a24a0c2047f19ff7c4e4507ff1b5 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 28 Apr 2018 00:34:52 +0200 Subject: [PATCH 22/67] http2: avoid bind and properly clean up in compat PR-URL: https://github.com/nodejs/node/pull/20374 Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat --- lib/internal/http2/compat.js | 78 +++++++++++++++++++++--------------- 1 file changed, 46 insertions(+), 32 deletions(-) diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 02b595b6a023f8..009bc7a2ac7ab6 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -7,7 +7,6 @@ const constants = binding.constants; const errors = require('internal/errors'); const { kSocket } = require('internal/http2/util'); -const kFinish = Symbol('finish'); const kBeginSend = Symbol('begin-send'); const kState = Symbol('state'); const kStream = Symbol('stream'); @@ -211,6 +210,27 @@ const proxySocketHandler = { } }; +function onStreamCloseRequest() { + const req = this[kRequest]; + + if (req === undefined) + return; + + const state = req[kState]; + state.closed = true; + + req.push(null); + // if the user didn't interact with incoming data and didn't pipe it, + // dump it for compatibility with http1 + if (!state.didRead && !req._readableState.resumeScheduled) + req.resume(); + + this[kProxySocket] = null; + this[kRequest] = undefined; + + req.emit('close'); +} + class Http2ServerRequest extends Readable { constructor(stream, headers, options, rawHeaders) { super(options); @@ -234,7 +254,7 @@ class Http2ServerRequest extends Readable { stream.on('end', onStreamEnd); stream.on('error', onStreamError); stream.on('aborted', onStreamAbortedRequest); - stream.on('close', this[kFinish].bind(this)); + stream.on('close', onStreamCloseRequest); this.on('pause', onRequestPause); this.on('resume', onRequestResume); } @@ -335,24 +355,30 @@ class Http2ServerRequest extends Readable { return; this[kStream].setTimeout(msecs, callback); } - - [kFinish]() { - const state = this[kState]; - if (state.closed) - return; - state.closed = true; - this.push(null); - this[kStream][kRequest] = undefined; - // if the user didn't interact with incoming data and didn't pipe it, - // dump it for compatibility with http1 - if (!state.didRead && !this._readableState.resumeScheduled) - this.resume(); - this.emit('close'); - } } function onStreamTrailersReady() { - this[kStream].sendTrailers(this[kTrailers]); + this.sendTrailers(this[kResponse][kTrailers]); +} + +function onStreamCloseResponse() { + const res = this[kResponse]; + + if (res === undefined) + return; + + const state = res[kState]; + + if (this.headRequest !== state.headRequest) + return; + + state.closed = true; + + this[kProxySocket] = null; + this[kResponse] = undefined; + + res.emit('finish'); + res.emit('close'); } class Http2ServerResponse extends Stream { @@ -373,8 +399,8 @@ class Http2ServerResponse extends Stream { this.writable = true; stream.on('drain', onStreamDrain); stream.on('aborted', onStreamAbortedResponse); - stream.on('close', this[kFinish].bind(this)); - stream.on('wantTrailers', onStreamTrailersReady.bind(this)); + stream.on('close', onStreamCloseResponse); + stream.on('wantTrailers', onStreamTrailersReady); } // User land modules such as finalhandler just check truthiness of this @@ -605,7 +631,7 @@ class Http2ServerResponse extends Stream { this.writeHead(this[kState].statusCode); if (isFinished) - this[kFinish](); + onStreamCloseResponse.call(stream); else stream.end(); } @@ -649,18 +675,6 @@ class Http2ServerResponse extends Stream { this[kStream].respond(headers, options); } - [kFinish]() { - const stream = this[kStream]; - const state = this[kState]; - if (state.closed || stream.headRequest !== state.headRequest) - return; - state.closed = true; - this[kProxySocket] = null; - stream[kResponse] = undefined; - this.emit('finish'); - this.emit('close'); - } - // TODO doesn't support callbacks writeContinue() { const stream = this[kStream]; From 9cf40c5753d1a9afbe24cb8a7f7c87211d6983ff Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 10 May 2018 20:45:44 -0700 Subject: [PATCH 23/67] test: improve reliability of http2-session-timeout Check actual expired time rather than relying on a number of calls to setTimeout() in test-http2-session-timeout more robust. PR-URL: https://github.com/nodejs/node/pull/20692 Fixes: https://github.com/nodejs/node/issues/20628 Reviewed-By: Anatoli Papirovski Reviewed-By: Trivikram Kamat Reviewed-By: Ruben Bridgewater Reviewed-By: James M Snell --- test/sequential/test-http2-session-timeout.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/test/sequential/test-http2-session-timeout.js b/test/sequential/test-http2-session-timeout.js index fce4570563c584..48e98998c700b6 100644 --- a/test/sequential/test-http2-session-timeout.js +++ b/test/sequential/test-http2-session-timeout.js @@ -7,7 +7,6 @@ const h2 = require('http2'); const serverTimeout = common.platformTimeout(200); const callTimeout = common.platformTimeout(20); -const minRuns = Math.ceil(serverTimeout / callTimeout) * 2; const mustNotCall = common.mustNotCall(); const server = h2.createServer(); @@ -21,9 +20,10 @@ server.listen(0, common.mustCall(() => { const url = `http://localhost:${port}`; const client = h2.connect(url); - makeReq(minRuns); + const startTime = process.hrtime(); + makeReq(); - function makeReq(attempts) { + function makeReq() { const request = client.request({ ':path': '/foobar', ':method': 'GET', @@ -34,12 +34,14 @@ server.listen(0, common.mustCall(() => { request.end(); request.on('end', () => { - if (attempts) { - setTimeout(() => makeReq(attempts - 1), callTimeout); + const diff = process.hrtime(startTime); + const milliseconds = (diff[0] * 1e3 + diff[1] / 1e6); + if (milliseconds < serverTimeout * 2) { + setTimeout(makeReq, callTimeout); } else { server.removeListener('timeout', mustNotCall); - client.close(); server.close(); + client.close(); } }); } From 2238e12ebade8d63503d67ba5d174b70061c72ed Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 9 May 2018 12:12:43 +0200 Subject: [PATCH 24/67] http2: fix end without read Adjust http2 behaviour to allow ending a stream even after some data comes in (when the user has no intention of reading that data). Also correctly end a stream when trailers are present. PR-URL: https://github.com/nodejs/node/pull/20621 Fixes: https://github.com/nodejs/node/issues/20060 Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Trivikram Kamat --- lib/internal/http2/compat.js | 10 +++-- lib/internal/http2/core.js | 12 ++--- .../test-http2-client-upload-reject.js | 15 ++++--- .../test-http2-compat-client-upload-reject.js | 44 +++++++++++++++++++ 4 files changed, 66 insertions(+), 15 deletions(-) create mode 100644 test/parallel/test-http2-compat-client-upload-reject.js diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 009bc7a2ac7ab6..9b2367c4c97a57 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -248,8 +248,6 @@ class Http2ServerRequest extends Readable { stream[kRequest] = this; // Pause the stream.. - stream.pause(); - stream.on('data', onStreamData); stream.on('trailers', onStreamTrailers); stream.on('end', onStreamEnd); stream.on('error', onStreamError); @@ -316,8 +314,12 @@ class Http2ServerRequest extends Readable { _read(nread) { const state = this[kState]; if (!state.closed) { - state.didRead = true; - process.nextTick(resumeStream, this[kStream]); + if (!state.didRead) { + state.didRead = true; + this[kStream].on('data', onStreamData); + } else { + process.nextTick(resumeStream, this[kStream]); + } } else { this.emit('error', new errors.Error('ERR_HTTP2_INVALID_STREAM')); } diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index a42e416851af32..5d70f436bbc676 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -300,11 +300,11 @@ function onStreamClose(code) { // Push a null so the stream can end whenever the client consumes // it completely. stream.push(null); - - // Same as net. - if (stream._readableState.length === 0) { - stream.read(0); - } + // If the client hasn't tried to consume the stream and there is no + // resume scheduled (which would indicate they would consume in the future), + // then just dump the incoming data so that the stream can be destroyed. + if (!stream[kState].didRead && !stream._readableState.resumeScheduled) + stream.resume(); } } @@ -1789,6 +1789,8 @@ class Http2Stream extends Duplex { const ret = this[kHandle].trailers(headersList); if (ret < 0) this.destroy(new NghttpError(ret)); + else + this[kMaybeDestroy](); } get closed() { diff --git a/test/parallel/test-http2-client-upload-reject.js b/test/parallel/test-http2-client-upload-reject.js index ece7cbdf233f1f..678114130e3dba 100644 --- a/test/parallel/test-http2-client-upload-reject.js +++ b/test/parallel/test-http2-client-upload-reject.js @@ -20,12 +20,15 @@ fs.readFile(loc, common.mustCall((err, data) => { const server = http2.createServer(); server.on('stream', common.mustCall((stream) => { - stream.on('close', common.mustCall(() => { - assert.strictEqual(stream.rstCode, 0); - })); - - stream.respond({ ':status': 400 }); - stream.end(); + // Wait for some data to come through. + setImmediate(() => { + stream.on('close', common.mustCall(() => { + assert.strictEqual(stream.rstCode, 0); + })); + + stream.respond({ ':status': 400 }); + stream.end(); + }); })); server.listen(0, common.mustCall(() => { diff --git a/test/parallel/test-http2-compat-client-upload-reject.js b/test/parallel/test-http2-compat-client-upload-reject.js new file mode 100644 index 00000000000000..e6a187cb12b264 --- /dev/null +++ b/test/parallel/test-http2-compat-client-upload-reject.js @@ -0,0 +1,44 @@ +'use strict'; + +// Verifies that uploading data from a client works + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); +const fs = require('fs'); +const fixtures = require('../common/fixtures'); + +const loc = fixtures.path('person-large.jpg'); + +assert(fs.existsSync(loc)); + +fs.readFile(loc, common.mustCall((err, data) => { + assert.ifError(err); + + const server = http2.createServer(common.mustCall((req, res) => { + setImmediate(() => { + res.writeHead(400); + res.end(); + }); + })); + + server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + + const req = client.request({ ':method': 'POST' }); + req.on('response', common.mustCall((headers) => { + assert.strictEqual(headers[':status'], 400); + })); + + req.resume(); + req.on('end', common.mustCall(() => { + server.close(); + client.close(); + })); + + const str = fs.createReadStream(loc); + str.pipe(req); + })); +})); From c9f8d6f45e2173bca2327fca53fb0b5de204aebe Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Thu, 10 May 2018 02:47:56 +0530 Subject: [PATCH 25/67] net,http2: refactor _write and _writev Refactor writable part (the _write and _writev functions) in net.Socket and http2.Http2Stream classes. Also involves adding a generic "WriteGeneric" method to the Http2Stream class based on net.Socket._writeGeneric, but behind a symbol. PR-URL: https://github.com/nodejs/node/pull/20643 Reviewed-By: Anatoli Papirovski Reviewed-By: Ruben Bridgewater --- lib/internal/http2/core.js | 67 ++++++++++++++------------------------ 1 file changed, 24 insertions(+), 43 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 5d70f436bbc676..106991dbd4224d 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -95,6 +95,7 @@ const kSession = Symbol('session'); const kState = Symbol('state'); const kType = Symbol('type'); const kUpdateTimer = Symbol('update-timer'); +const kWriteGeneric = Symbol('write-generic'); const kDefaultSocketTimeout = 2 * 60 * 1000; @@ -1635,13 +1636,16 @@ class Http2Stream extends Duplex { 'bug in Node.js'); } - _write(data, encoding, cb) { + [kWriteGeneric](writev, data, encoding, cb) { // When the Http2Stream is first created, it is corked until the // handle and the stream ID is assigned. However, if the user calls // uncork() before that happens, the Duplex will attempt to pass // writes through. Those need to be queued up here. if (this.pending) { - this.once('ready', this._write.bind(this, data, encoding, cb)); + this.once( + 'ready', + this[kWriteGeneric].bind(this, writev, data, encoding, cb) + ); return; } @@ -1665,53 +1669,30 @@ class Http2Stream extends Duplex { req.callback = cb; req.oncomplete = afterDoStreamWrite; req.async = false; - const err = createWriteReq(req, handle, data, encoding); + + let err; + if (writev) { + const chunks = new Array(data.length << 1); + for (var i = 0; i < data.length; i++) { + const entry = data[i]; + chunks[i * 2] = entry.chunk; + chunks[i * 2 + 1] = entry.encoding; + } + err = handle.writev(req, chunks); + } else { + err = createWriteReq(req, handle, data, encoding); + } if (err) return this.destroy(errors.errnoException(err, 'write', req.error), cb); trackWriteState(this, req.bytes); } - _writev(data, cb) { - // When the Http2Stream is first created, it is corked until the - // handle and the stream ID is assigned. However, if the user calls - // uncork() before that happens, the Duplex will attempt to pass - // writes through. Those need to be queued up here. - if (this.pending) { - this.once('ready', this._writev.bind(this, data, cb)); - return; - } - - // If the stream has been destroyed, there's nothing else we can do - // because the handle has been destroyed. This should only be an - // issue if a write occurs before the 'ready' event in the case where - // the duplex is uncorked before the stream is ready to go. In that - // case, drop the data on the floor. An error should have already been - // emitted. - if (this.destroyed) - return; - - this[kUpdateTimer](); - - if (!this.headersSent) - this[kProceed](); + _write(data, encoding, cb) { + this[kWriteGeneric](false, data, encoding, cb); + } - const handle = this[kHandle]; - const req = new WriteWrap(); - req.stream = this[kID]; - req.handle = handle; - req.callback = cb; - req.oncomplete = afterDoStreamWrite; - req.async = false; - const chunks = new Array(data.length << 1); - for (var i = 0; i < data.length; i++) { - const entry = data[i]; - chunks[i * 2] = entry.chunk; - chunks[i * 2 + 1] = entry.encoding; - } - const err = handle.writev(req, chunks); - if (err) - return this.destroy(errors.errnoException(err, 'write', req.error), cb); - trackWriteState(this, req.bytes); + _writev(data, cb) { + this[kWriteGeneric](true, data, '', cb); } _final(cb) { From 4ef2393f6371b16e3adf61c5e535b00fbc5f159e Mon Sep 17 00:00:00 2001 From: Keita Akutsu Date: Sun, 20 May 2018 14:01:44 +0900 Subject: [PATCH 26/67] doc: fix typo in http2.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/20843 Reviewed-By: Colin Ihrig Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: Vse Mozhet Byt Reviewed-By: Tobias Nießen --- doc/api/http2.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index 7c162d1062d2c8..479b6bb48310ee 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -3123,7 +3123,7 @@ added: v8.4.0 --> Call [`http2stream.pushStream()`][] with the given headers, and wraps the -given newly created [`Http2Stream`] on `Http2ServerRespose`. +given newly created [`Http2Stream`] on `Http2ServerResponse`. The callback will be called with an error with code `ERR_HTTP2_STREAM_CLOSED` if the stream is closed. From 2151bf3f0b4d062ee320aad118a3a5b953e86796 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 17 May 2018 23:03:15 +0400 Subject: [PATCH 27/67] http2: fix several serious bugs Currently http2 does not properly submit GOAWAY frames when a session is being destroyed. It also doesn't properly handle when the other party severs the connection after sending a GOAWAY frame, even though it should. Edge, IE & Safari are currently unable to handle empty TRAILERS frames despite them being correctly to spec. Instead send an empty DATA frame with END_STREAM flag in those situations. Fix and adjust several flaky and/or incorrect tests. PR-URL: https://github.com/nodejs/node/pull/20772 Fixes: https://github.com/nodejs/node/issues/20705 Fixes: https://github.com/nodejs/node/issues/20750 Fixes: https://github.com/nodejs/node/issues/20850 Reviewed-By: Matteo Collina Reviewed-By: Rich Trott Reviewed-By: James M Snell --- doc/api/http2.md | 4 + lib/internal/http2/core.js | 109 +++++++++++------- src/node_http2.cc | 50 +++++--- src/node_http2.h | 8 +- test/parallel/test-http2-client-destroy.js | 8 +- .../test-http2-no-wanttrailers-listener.js | 5 +- .../test-http2-server-close-callback.js | 19 +-- .../test-http2-server-sessionerror.js | 13 ++- ...st-http2-server-shutdown-options-errors.js | 6 - .../test-http2-server-socket-destroy.js | 10 +- ...est-http2-server-stream-session-destroy.js | 8 -- test/parallel/test-http2-server-timeout.js | 10 +- test/parallel/test-http2-too-many-settings.js | 7 +- test/parallel/test-http2-trailers.js | 3 +- 14 files changed, 145 insertions(+), 115 deletions(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index 479b6bb48310ee..3ffb41e1402ca2 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -884,6 +884,10 @@ The `'trailers'` event is emitted when a block of headers associated with trailing header fields is received. The listener callback is passed the [HTTP/2 Headers Object][] and flags associated with the headers. +Note that this event might not be emitted if `http2stream.end()` is called +before trailers are received and the incoming data is not being read or +listened for. + ```js stream.on('trailers', (headers, flags) => { console.log(headers); diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 106991dbd4224d..5a284a60771f26 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -292,20 +292,25 @@ function onStreamClose(code) { tryClose(stream[kState].fd); // Defer destroy we actually emit end. - if (stream._readableState.endEmitted || code !== NGHTTP2_NO_ERROR) { + if (!stream.readable || code !== NGHTTP2_NO_ERROR) { // If errored or ended, we can destroy immediately. - stream[kMaybeDestroy](null, code); + stream[kMaybeDestroy](code); } else { // Wait for end to destroy. stream.on('end', stream[kMaybeDestroy]); // Push a null so the stream can end whenever the client consumes // it completely. stream.push(null); - // If the client hasn't tried to consume the stream and there is no - // resume scheduled (which would indicate they would consume in the future), - // then just dump the incoming data so that the stream can be destroyed. - if (!stream[kState].didRead && !stream._readableState.resumeScheduled) + + // If the user hasn't tried to consume the stream (and this is a server + // session) then just dump the incoming data so that the stream can + // be destroyed. + if (stream[kSession][kType] === NGHTTP2_SESSION_SERVER && + !stream[kState].didRead && + stream._readableState.flowing === null) stream.resume(); + else + stream.read(0); } } @@ -330,7 +335,7 @@ function onStreamRead(nread, buf) { `${sessionName(stream[kSession][kType])}]: ending readable.`); // defer this until we actually emit end - if (stream._readableState.endEmitted) { + if (!stream.readable) { stream[kMaybeDestroy](); } else { stream.on('end', stream[kMaybeDestroy]); @@ -421,7 +426,7 @@ function onGoawayData(code, lastStreamID, buf) { // condition on this side of the session that caused the // shutdown. session.destroy(new errors.Error('ERR_HTTP2_SESSION_ERROR', code), - { errorCode: NGHTTP2_NO_ERROR }); + NGHTTP2_NO_ERROR); } } @@ -772,6 +777,21 @@ function emitClose(self, error) { self.emit('close'); } +function finishSessionDestroy(session, error) { + const socket = session[kSocket]; + if (!socket.destroyed) + socket.destroy(error); + + session[kProxySocket] = undefined; + session[kSocket] = undefined; + session[kHandle] = undefined; + socket[kSession] = undefined; + socket[kServer] = undefined; + + // Finally, emit the close and error events (if necessary) on next tick. + process.nextTick(emitClose, session, error); +} + // Upon creation, the Http2Session takes ownership of the socket. The session // may not be ready to use immediately if the socket is not yet fully connected. // In that case, the Http2Session will wait for the socket to connect. Once @@ -828,6 +848,8 @@ class Http2Session extends EventEmitter { this[kState] = { flags: SESSION_FLAGS_PENDING, + goawayCode: null, + goawayLastStreamID: null, streams: new Map(), pendingStreams: new Set(), pendingAck: 0, @@ -1130,25 +1152,13 @@ class Http2Session extends EventEmitter { if (handle !== undefined) handle.destroy(code, socket.destroyed); - // If there is no error, use setImmediate to destroy the socket on the + // If the socket is alive, use setImmediate to destroy the session on the // next iteration of the event loop in order to give data time to transmit. // Otherwise, destroy immediately. - if (!socket.destroyed) { - if (!error) { - setImmediate(socket.destroy.bind(socket)); - } else { - socket.destroy(error); - } - } - - this[kProxySocket] = undefined; - this[kSocket] = undefined; - this[kHandle] = undefined; - socket[kSession] = undefined; - socket[kServer] = undefined; - - // Finally, emit the close and error events (if necessary) on next tick. - process.nextTick(emitClose, this, error); + if (!socket.destroyed) + setImmediate(finishSessionDestroy, this, error); + else + finishSessionDestroy(this, error); } // Closing the session will: @@ -1422,11 +1432,8 @@ function afterDoStreamWrite(status, handle, req) { } function streamOnResume() { - if (!this.destroyed && !this.pending) { - if (!this[kState].didRead) - this[kState].didRead = true; + if (!this.destroyed) this[kHandle].readStart(); - } } function streamOnPause() { @@ -1441,6 +1448,16 @@ function afterShutdown() { stream[kMaybeDestroy](); } +function finishSendTrailers(stream, headersList) { + stream[kState].flags &= ~STREAM_FLAGS_HAS_TRAILERS; + + const ret = stream[kHandle].trailers(headersList); + if (ret < 0) + stream.destroy(new NghttpError(ret)); + else + stream[kMaybeDestroy](); +} + function closeStream(stream, code, shouldSubmitRstStream = true) { const state = stream[kState]; state.flags |= STREAM_FLAGS_CLOSED; @@ -1502,6 +1519,10 @@ class Http2Stream extends Duplex { this[kSession] = session; session[kState].pendingStreams.add(this); + // Allow our logic for determining whether any reads have happened to + // work in all situations. This is similar to what we do in _http_incoming. + this._readableState.readingMore = true; + this[kState] = { didRead: false, flags: STREAM_FLAGS_PENDING, @@ -1510,7 +1531,6 @@ class Http2Stream extends Duplex { trailersReady: false }; - this.on('resume', streamOnResume); this.on('pause', streamOnPause); } @@ -1717,6 +1737,10 @@ class Http2Stream extends Duplex { this.push(null); return; } + if (!this[kState].didRead) { + this._readableState.readingMore = false; + this[kState].didRead = true; + } if (!this.pending) { streamOnResume.call(this); } else { @@ -1765,13 +1789,8 @@ class Http2Stream extends Duplex { throw headersList; this[kSentTrailers] = headers; - this[kState].flags &= ~STREAM_FLAGS_HAS_TRAILERS; - - const ret = this[kHandle].trailers(headersList); - if (ret < 0) - this.destroy(new NghttpError(ret)); - else - this[kMaybeDestroy](); + // Send the trailers in setImmediate so we don't do it on nghttp2 stack. + setImmediate(finishSendTrailers, this, headersList); } get closed() { @@ -1861,15 +1880,15 @@ class Http2Stream extends Duplex { } // The Http2Stream can be destroyed if it has closed and if the readable // side has received the final chunk. - [kMaybeDestroy](error, code = NGHTTP2_NO_ERROR) { - if (error || code !== NGHTTP2_NO_ERROR) { - this.destroy(error); + [kMaybeDestroy](code = NGHTTP2_NO_ERROR) { + if (code !== NGHTTP2_NO_ERROR) { + this.destroy(); return; } // TODO(mcollina): remove usage of _*State properties - if (this._writableState.ended && this._writableState.pendingcb === 0) { - if (this._readableState.ended && this.closed) { + if (!this.writable) { + if (!this.readable && this.closed) { this.destroy(); return; } @@ -1882,7 +1901,7 @@ class Http2Stream extends Duplex { this[kSession][kType] === NGHTTP2_SESSION_SERVER && !(state.flags & STREAM_FLAGS_HAS_TRAILERS) && !state.didRead && - !this._readableState.resumeScheduled) { + this._readableState.flowing === null) { this.close(); } } @@ -2445,6 +2464,10 @@ Object.defineProperty(Http2Session.prototype, 'setTimeout', setTimeout); function socketOnError(error) { const session = this[kSession]; if (session !== undefined) { + // We can ignore ECONNRESET after GOAWAY was received as there's nothing + // we can do and the other side is fully within its rights to do so. + if (error.code === 'ECONNRESET' && session[kState].goawayCode !== null) + return session.destroy(); debug(`Http2Session ${sessionName(session[kType])}: socket error [` + `${error.message}]`); session.destroy(error); diff --git a/src/node_http2.cc b/src/node_http2.cc index 237e5c01b91696..4630e1228307c6 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -631,9 +631,9 @@ inline void Http2Session::EmitStatistics() { void Http2Session::Close(uint32_t code, bool socket_closed) { DEBUG_HTTP2SESSION(this, "closing session"); - if (flags_ & SESSION_STATE_CLOSED) + if (flags_ & SESSION_STATE_CLOSING) return; - flags_ |= SESSION_STATE_CLOSED; + flags_ |= SESSION_STATE_CLOSING; // Stop reading on the i/o stream if (stream_ != nullptr) @@ -641,16 +641,18 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { // If the socket is not closed, then attempt to send a closing GOAWAY // frame. There is no guarantee that this GOAWAY will be received by - // the peer but the HTTP/2 spec recommends sendinng it anyway. We'll + // the peer but the HTTP/2 spec recommends sending it anyway. We'll // make a best effort. if (!socket_closed) { - Http2Scope h2scope(this); DEBUG_HTTP2SESSION2(this, "terminating session with code %d", code); CHECK_EQ(nghttp2_session_terminate_session(session_, code), 0); + SendPendingData(); } else { Unconsume(); } + flags_ |= SESSION_STATE_CLOSED; + // If there are outstanding pings, those will need to be canceled, do // so on the next iteration of the event loop to avoid calling out into // javascript since this may be called during garbage collection. @@ -1387,25 +1389,32 @@ void Http2Session::MaybeScheduleWrite() { } } +void Http2Session::MaybeStopReading() { + int want_read = nghttp2_session_want_read(session_); + DEBUG_HTTP2SESSION2(this, "wants read? %d", want_read); + if (want_read == 0) + stream_->ReadStop(); +} + // Unset the sending state, finish up all current writes, and reset // storage for data and metadata that was associated with these writes. void Http2Session::ClearOutgoing(int status) { CHECK_NE(flags_ & SESSION_STATE_SENDING, 0); + flags_ &= ~SESSION_STATE_SENDING; + if (outgoing_buffers_.size() > 0) { outgoing_storage_.clear(); - for (const nghttp2_stream_write& wr : outgoing_buffers_) { + std::vector current_outgoing_buffers_; + current_outgoing_buffers_.swap(outgoing_buffers_); + for (const nghttp2_stream_write& wr : current_outgoing_buffers_) { WriteWrap* wrap = wr.req_wrap; if (wrap != nullptr) wrap->Done(status); } - - outgoing_buffers_.clear(); } - flags_ &= ~SESSION_STATE_SENDING; - // Now that we've finished sending queued data, if there are any pending // RstStreams we should try sending again and then flush them one by one. if (pending_rst_streams_.size() > 0) { @@ -1525,8 +1534,7 @@ uint8_t Http2Session::SendPendingData() { req->Dispose(); } - DEBUG_HTTP2SESSION2(this, "wants data in return? %d", - nghttp2_session_want_read(session_)); + MaybeStopReading(); return 0; } @@ -1691,8 +1699,7 @@ void Http2Session::OnStreamReadImpl(ssize_t nread, }; session->MakeCallback(env->error_string(), arraysize(argv), argv); } else { - DEBUG_HTTP2SESSION2(session, "processed %d bytes. wants more? %d", ret, - nghttp2_session_want_read(**session)); + session->MaybeStopReading(); } } @@ -1928,6 +1935,7 @@ void Http2Stream::OnTrailers() { HandleScope scope(isolate); Local context = env()->context(); Context::Scope context_scope(context); + flags_ &= ~NGHTTP2_STREAM_FLAG_TRAILERS; MakeCallback(env()->ontrailers_string(), 0, nullptr); } @@ -1936,7 +1944,16 @@ int Http2Stream::SubmitTrailers(nghttp2_nv* nva, size_t len) { CHECK(!this->IsDestroyed()); Http2Scope h2scope(this); DEBUG_HTTP2STREAM2(this, "sending %d trailers", len); - int ret = nghttp2_submit_trailer(**session_, id_, nva, len); + int ret; + // Sending an empty trailers frame poses problems in Safari, Edge & IE. + // Instead we can just send an empty data frame with NGHTTP2_FLAG_END_STREAM + // to indicate that the stream is ready to be closed. + if (len == 0) { + Http2Stream::Provider::Stream prov(this, 0); + ret = nghttp2_submit_data(**session_, NGHTTP2_FLAG_END_STREAM, id_, *prov); + } else { + ret = nghttp2_submit_trailer(**session_, id_, nva, len); + } CHECK_NE(ret, NGHTTP2_ERR_NOMEM); return ret; } @@ -2562,8 +2579,7 @@ void Http2Stream::Info(const FunctionCallbackInfo& args) { Headers list(isolate, context, headers); args.GetReturnValue().Set(stream->SubmitInfo(*list, list.length())); - DEBUG_HTTP2STREAM2(stream, "%d informational headers sent", - headers->Length()); + DEBUG_HTTP2STREAM2(stream, "%d informational headers sent", list.length()); } // Submits trailing headers on the Http2Stream @@ -2578,7 +2594,7 @@ void Http2Stream::Trailers(const FunctionCallbackInfo& args) { Headers list(isolate, context, headers); args.GetReturnValue().Set(stream->SubmitTrailers(*list, list.length())); - DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", headers->Length()); + DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", list.length()); } // Grab the numeric id of the Http2Stream diff --git a/src/node_http2.h b/src/node_http2.h index 972c08a2cd98ba..009c05b731eee8 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -433,7 +433,8 @@ enum session_state_flags { SESSION_STATE_HAS_SCOPE = 0x1, SESSION_STATE_WRITE_SCHEDULED = 0x2, SESSION_STATE_CLOSED = 0x4, - SESSION_STATE_SENDING = 0x8, + SESSION_STATE_CLOSING = 0x8, + SESSION_STATE_SENDING = 0x10, }; // This allows for 4 default-sized frames with their frame headers @@ -624,7 +625,7 @@ class Http2Stream : public AsyncWrap, inline bool IsClosed() const { return flags_ & NGHTTP2_STREAM_FLAG_CLOSED; - } + } inline bool HasTrailers() const { return flags_ & NGHTTP2_STREAM_FLAG_TRAILERS; @@ -846,6 +847,9 @@ class Http2Session : public AsyncWrap { // Schedule a write if nghttp2 indicates it wants to write to the socket. void MaybeScheduleWrite(); + // Stop reading if nghttp2 doesn't want to anymore. + void MaybeStopReading(); + // Returns pointer to the stream, or nullptr if stream does not exist inline Http2Stream* FindStream(int32_t id); diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index 6238363511a791..43fc6819e21f7a 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/test/parallel/test-http2-client-destroy.js @@ -109,9 +109,6 @@ const Countdown = require('../common/countdown'); server.listen(0, common.mustCall(() => { const client = h2.connect(`http://localhost:${server.address().port}`); - // On some platforms (e.g. windows), an ECONNRESET may occur at this - // point -- or it may not. Do not make this a mustCall - client.on('error', () => {}); client.on('close', () => { server.close(); @@ -119,10 +116,7 @@ const Countdown = require('../common/countdown'); client.destroy(); }); - const req = client.request(); - // On some platforms (e.g. windows), an ECONNRESET may occur at this - // point -- or it may not. Do not make this a mustCall - req.on('error', () => {}); + client.request(); })); } diff --git a/test/parallel/test-http2-no-wanttrailers-listener.js b/test/parallel/test-http2-no-wanttrailers-listener.js index 7ba25d0e491e15..87bc21df48aa2c 100644 --- a/test/parallel/test-http2-no-wanttrailers-listener.js +++ b/test/parallel/test-http2-no-wanttrailers-listener.js @@ -3,7 +3,6 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); -const assert = require('assert'); const h2 = require('http2'); const server = h2.createServer(); @@ -25,9 +24,7 @@ server.on('listening', common.mustCall(function() { const client = h2.connect(`http://localhost:${this.address().port}`); const req = client.request(); req.resume(); - req.on('trailers', common.mustCall((headers) => { - assert.strictEqual(Object.keys(headers).length, 0); - })); + req.on('trailers', common.mustNotCall()); req.on('close', common.mustCall(() => { server.close(); client.close(); diff --git a/test/parallel/test-http2-server-close-callback.js b/test/parallel/test-http2-server-close-callback.js index 66887aa62bebe5..f822d8a4a92d78 100644 --- a/test/parallel/test-http2-server-close-callback.js +++ b/test/parallel/test-http2-server-close-callback.js @@ -4,21 +4,24 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); +const Countdown = require('../common/countdown'); const http2 = require('http2'); const server = http2.createServer(); +let session; + +const countdown = new Countdown(2, () => { + server.close(common.mustCall()); + session.destroy(); +}); + server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); - client.on('error', (err) => { - if (err.code !== 'ECONNRESET') - throw err; - }); + client.on('connect', common.mustCall(() => countdown.dec())); })); server.on('session', common.mustCall((s) => { - setImmediate(() => { - server.close(common.mustCall()); - s.destroy(); - }); + session = s; + countdown.dec(); })); diff --git a/test/parallel/test-http2-server-sessionerror.js b/test/parallel/test-http2-server-sessionerror.js index 525eb2e6efd11a..c50352fcc35c28 100644 --- a/test/parallel/test-http2-server-sessionerror.js +++ b/test/parallel/test-http2-server-sessionerror.js @@ -35,14 +35,17 @@ server.on('session', common.mustCall((session) => { server.listen(0, common.mustCall(() => { const url = `http://localhost:${server.address().port}`; http2.connect(url) - // An ECONNRESET error may occur depending on the platform (due largely - // to differences in the timing of socket closing). Do not wrap this in - // a common must call. - .on('error', () => {}) + .on('error', common.expectsError({ + code: 'ERR_HTTP2_SESSION_ERROR', + message: 'Session closed with error code 2', + })) .on('close', () => { server.removeAllListeners('error'); http2.connect(url) - .on('error', () => {}) + .on('error', common.expectsError({ + code: 'ERR_HTTP2_SESSION_ERROR', + message: 'Session closed with error code 2', + })) .on('close', () => server.close()); }); })); diff --git a/test/parallel/test-http2-server-shutdown-options-errors.js b/test/parallel/test-http2-server-shutdown-options-errors.js index 2aedec1140701a..94733b199366db 100644 --- a/test/parallel/test-http2-server-shutdown-options-errors.js +++ b/test/parallel/test-http2-server-shutdown-options-errors.js @@ -54,13 +54,7 @@ server.listen( 0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); - // On certain operating systems, an ECONNRESET may occur. We do not need - // to test for it here. Do not make this a mustCall - client.on('error', () => {}); const req = client.request(); - // On certain operating systems, an ECONNRESET may occur. We do not need - // to test for it here. Do not make this a mustCall - req.on('error', () => {}); req.resume(); req.on('close', common.mustCall(() => { client.close(); diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js index 03afc1957b8af4..99595aeb63004d 100644 --- a/test/parallel/test-http2-server-socket-destroy.js +++ b/test/parallel/test-http2-server-socket-destroy.js @@ -41,14 +41,20 @@ server.on('listening', common.mustCall(() => { // The client may have an ECONNRESET error here depending on the operating // system, due mainly to differences in the timing of socket closing. Do // not wrap this in a common mustCall. - client.on('error', () => {}); + client.on('error', (err) => { + if (err.code !== 'ECONNRESET') + throw err; + }); client.on('close', common.mustCall()); const req = client.request({ ':method': 'POST' }); // The client may have an ECONNRESET error here depending on the operating // system, due mainly to differences in the timing of socket closing. Do // not wrap this in a common mustCall. - req.on('error', () => {}); + req.on('error', (err) => { + if (err.code !== 'ECONNRESET') + throw err; + }); req.on('aborted', common.mustCall()); req.resume(); diff --git a/test/parallel/test-http2-server-stream-session-destroy.js b/test/parallel/test-http2-server-stream-session-destroy.js index e70630fe0b1351..6d8de4ba5f7618 100644 --- a/test/parallel/test-http2-server-stream-session-destroy.js +++ b/test/parallel/test-http2-server-stream-session-destroy.js @@ -39,16 +39,8 @@ server.on('stream', common.mustCall((stream) => { server.listen(0, common.mustCall(() => { const client = h2.connect(`http://localhost:${server.address().port}`); - client.on('error', (err) => { - if (err.code !== 'ECONNRESET') - throw err; - }); const req = client.request(); req.resume(); req.on('end', common.mustCall()); req.on('close', common.mustCall(() => server.close(common.mustCall()))); - req.on('error', (err) => { - if (err.code !== 'ECONNRESET') - throw err; - }); })); diff --git a/test/parallel/test-http2-server-timeout.js b/test/parallel/test-http2-server-timeout.js index 581a409ce9171d..88fc4eab2e08c0 100755 --- a/test/parallel/test-http2-server-timeout.js +++ b/test/parallel/test-http2-server-timeout.js @@ -6,10 +6,10 @@ if (!common.hasCrypto) const http2 = require('http2'); const server = http2.createServer(); -server.setTimeout(common.platformTimeout(1)); +server.setTimeout(common.platformTimeout(50)); const onServerTimeout = common.mustCall((session) => { - session.close(() => session.destroy()); + session.close(); }); server.on('stream', common.mustNotCall()); @@ -18,14 +18,8 @@ server.once('timeout', onServerTimeout); server.listen(0, common.mustCall(() => { const url = `http://localhost:${server.address().port}`; const client = http2.connect(url); - // Because of the timeout, an ECONRESET error may or may not happen here. - // Keep this as a non-op and do not use common.mustCall() - client.on('error', () => {}); client.on('close', common.mustCall(() => { const client2 = http2.connect(url); - // Because of the timeout, an ECONRESET error may or may not happen here. - // Keep this as a non-op and do not use common.mustCall() - client2.on('error', () => {}); client2.on('close', common.mustCall(() => server.close())); })); })); diff --git a/test/parallel/test-http2-too-many-settings.js b/test/parallel/test-http2-too-many-settings.js index 0302fe623da07c..acfd73ada68416 100644 --- a/test/parallel/test-http2-too-many-settings.js +++ b/test/parallel/test-http2-too-many-settings.js @@ -29,9 +29,10 @@ function doTest(session) { server.listen(0, common.mustCall(() => { const client = h2.connect(`http://localhost:${server.address().port}`); - // On some operating systems, an ECONNRESET error may be emitted. - // On others it won't be. Do not make this a mustCall - client.on('error', () => {}); + client.on('error', common.expectsError({ + code: 'ERR_HTTP2_SESSION_ERROR', + message: 'Session closed with error code 2', + })); client.on('close', common.mustCall(() => server.close())); })); } diff --git a/test/parallel/test-http2-trailers.js b/test/parallel/test-http2-trailers.js index 2b7f6158ca3951..bdc0931157ad58 100644 --- a/test/parallel/test-http2-trailers.js +++ b/test/parallel/test-http2-trailers.js @@ -18,6 +18,7 @@ server.on('stream', common.mustCall(onStream)); function onStream(stream, headers, flags) { stream.on('trailers', common.mustCall((headers) => { assert.strictEqual(headers[trailerKey], trailerValue); + stream.end(body); })); stream.respond({ 'content-type': 'text/html', @@ -41,8 +42,6 @@ function onStream(stream, headers, flags) { type: Error } ); - - stream.end(body); } server.listen(0); From e825eed136f39bd53d6f6b23c1f368c36238a553 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 21 May 2018 11:42:32 +0400 Subject: [PATCH 28/67] test: fix flaky http2-session-unref It's possible for the connections to take too long and since the server is already unrefed, the process will just exit. Instead adjust the test so that server unref only happens after all sessions have been successfuly established and unrefed. That still tests the same condition but will not fail under load. PR-URL: https://github.com/nodejs/node/pull/20772 Fixes: https://github.com/nodejs/node/issues/20705 Fixes: https://github.com/nodejs/node/issues/20750 Fixes: https://github.com/nodejs/node/issues/20850 Reviewed-By: Matteo Collina Reviewed-By: Rich Trott Reviewed-By: James M Snell --- test/parallel/test-http2-session-unref.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-http2-session-unref.js b/test/parallel/test-http2-session-unref.js index 465f01d0921f25..0381971c0eace5 100644 --- a/test/parallel/test-http2-session-unref.js +++ b/test/parallel/test-http2-session-unref.js @@ -9,16 +9,20 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); const http2 = require('http2'); +const Countdown = require('../common/countdown'); const makeDuplexPair = require('../common/duplexpair'); const server = http2.createServer(); const { clientSide, serverSide } = makeDuplexPair(); +const counter = new Countdown(3, () => server.unref()); + // 'session' event should be emitted 3 times: // - the vanilla client // - the destroyed client // - manual 'connection' event emission with generic Duplex stream server.on('session', common.mustCallAtLeast((session) => { + counter.dec(); session.unref(); }, 3)); @@ -54,6 +58,3 @@ server.listen(0, common.mustCall(() => { } })); server.emit('connection', serverSide); -server.unref(); - -setTimeout(common.mustNotCall(() => {}), 1000).unref(); From 830cc57733be9766fe6638a8019ca6152b89df4c Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Mon, 28 May 2018 09:20:51 +0200 Subject: [PATCH 29/67] http2: delay closing stream Delay automatically closing the stream with setImmediate in order to allow any pushStreams to be sent first. PR-URL: https://github.com/nodejs/node/pull/20997 Fixes: https://github.com/nodejs/node/issues/20992 Reviewed-By: James M Snell --- lib/internal/http2/core.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 5a284a60771f26..4db1b9ea6e0720 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1902,7 +1902,10 @@ class Http2Stream extends Duplex { !(state.flags & STREAM_FLAGS_HAS_TRAILERS) && !state.didRead && this._readableState.flowing === null) { - this.close(); + // By using setImmediate we allow pushStreams to make it through + // before the stream is officially closed. This prevents a bug + // in most browsers where those pushStreams would be rejected. + setImmediate(this.close.bind(this)); } } } @@ -2137,7 +2140,7 @@ class ServerHttp2Stream extends Http2Stream { let headRequest = false; if (headers[HTTP2_HEADER_METHOD] === HTTP2_METHOD_HEAD) headRequest = options.endStream = true; - options.readable = !options.endStream; + options.readable = false; const headersList = mapToHeaders(headers); if (!Array.isArray(headersList)) From 21f3e27e2140f6dcbe76d7ff13d3dc37c2a0b025 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Tue, 29 May 2018 19:07:02 +0200 Subject: [PATCH 30/67] http2: force through RST_STREAM in destroy If still needed, force through RST_STREAM in Http2Stream#destroy calls, so that nghttp2 can wrap up properly and doesn't continue trying to read & write data to the stream. PR-URL: https://github.com/nodejs/node/pull/21016 Fixes: https://github.com/nodejs/node/issues/21008 Reviewed-By: Benjamin Gruenbaum Reviewed-By: James M Snell Reviewed-By: Anna Henningsen --- lib/internal/http2/core.js | 15 ++++--- src/node_http2.cc | 2 + src/node_http2.h | 7 ++++ .../test-http2-large-write-destroy.js | 40 +++++++++++++++++++ 4 files changed, 59 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-http2-large-write-destroy.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 4db1b9ea6e0720..39430edee6ff4f 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -286,7 +286,7 @@ function onStreamClose(code) { `${sessionName(stream[kSession][kType])}]: closed with code ${code}`); if (!stream.closed) - closeStream(stream, code, false); + closeStream(stream, code, kNoRstStream); if (stream[kState].fd !== undefined) tryClose(stream[kState].fd); @@ -1458,7 +1458,11 @@ function finishSendTrailers(stream, headersList) { stream[kMaybeDestroy](); } -function closeStream(stream, code, shouldSubmitRstStream = true) { +const kNoRstStream = 0; +const kSubmitRstStream = 1; +const kForceRstStream = 2; + +function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) { const state = stream[kState]; state.flags |= STREAM_FLAGS_CLOSED; state.rstCode = code; @@ -1481,9 +1485,10 @@ function closeStream(stream, code, shouldSubmitRstStream = true) { stream.end(); } - if (shouldSubmitRstStream) { + if (rstStreamStatus !== kNoRstStream) { const finishFn = finishCloseStream.bind(stream, code); - if (!ending || finished || code !== NGHTTP2_NO_ERROR) + if (!ending || finished || code !== NGHTTP2_NO_ERROR || + rstStreamStatus === kForceRstStream) finishFn(); else stream.once('finish', finishFn); @@ -1845,7 +1850,7 @@ class Http2Stream extends Duplex { const hasHandle = handle !== undefined; if (!this.closed) - closeStream(this, code, hasHandle); + closeStream(this, code, hasHandle ? kForceRstStream : kNoRstStream); this.push(null); if (hasHandle) { diff --git a/src/node_http2.cc b/src/node_http2.cc index 4630e1228307c6..4eb35330fd42e9 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1839,6 +1839,8 @@ inline void Http2Stream::Destroy() { // Do nothing if this stream instance is already destroyed if (IsDestroyed()) return; + if (session_->HasPendingRstStream(id_)) + FlushRstStream(); flags_ |= NGHTTP2_STREAM_FLAG_DESTROYED; DEBUG_HTTP2STREAM(this, "destroying stream"); diff --git a/src/node_http2.h b/src/node_http2.h index 009c05b731eee8..f054ff9e9d6a26 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -9,6 +9,7 @@ #include "stream_base-inl.h" #include "string_bytes.h" +#include #include namespace node { @@ -880,6 +881,12 @@ class Http2Session : public AsyncWrap { pending_rst_streams_.emplace_back(stream_id); } + inline bool HasPendingRstStream(int32_t stream_id) { + return pending_rst_streams_.end() != std::find(pending_rst_streams_.begin(), + pending_rst_streams_.end(), + stream_id); + } + static void OnStreamAllocImpl(size_t suggested_size, uv_buf_t* buf, void* ctx); diff --git a/test/parallel/test-http2-large-write-destroy.js b/test/parallel/test-http2-large-write-destroy.js new file mode 100644 index 00000000000000..24c0a055cc943f --- /dev/null +++ b/test/parallel/test-http2-large-write-destroy.js @@ -0,0 +1,40 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const fixtures = require('../common/fixtures'); +const http2 = require('http2'); + +// This test will result in a crash due to a missed CHECK in C++ or +// a straight-up segfault if the C++ doesn't send RST_STREAM through +// properly when calling destroy. + +const content = Buffer.alloc(60000, 0x44); + +const server = http2.createSecureServer({ + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem') +}); +server.on('stream', common.mustCall((stream) => { + stream.respond({ + 'Content-Type': 'application/octet-stream', + 'Content-Length': (content.length.toString() * 2), + 'Vary': 'Accept-Encoding' + }, { waitForTrailers: true }); + + stream.write(content); + stream.destroy(); +})); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`https://localhost:${server.address().port}`, + { rejectUnauthorized: false }); + + const req = client.request({ ':path': '/' }); + req.end(); + + req.on('close', common.mustCall(() => { + client.close(); + server.close(); + })); +})); From b53a75344977dd718d952c48bf5dc499c816c2e4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Jun 2018 16:01:58 +0200 Subject: [PATCH 31/67] test: minor adjustments to test-http2-respond-file PR-URL: https://github.com/nodejs/node/pull/21098 Reviewed-By: Anatoli Papirovski Reviewed-By: Rich Trott Reviewed-By: Jon Moss Reviewed-By: Richard Lau --- test/parallel/test-http2-respond-file.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-http2-respond-file.js b/test/parallel/test-http2-respond-file.js index 9ad8e7a69648dc..1c10ceb4350723 100644 --- a/test/parallel/test-http2-respond-file.js +++ b/test/parallel/test-http2-respond-file.js @@ -19,7 +19,7 @@ const data = fs.readFileSync(fname); const stat = fs.statSync(fname); const server = http2.createServer(); -server.on('stream', (stream) => { +server.on('stream', common.mustCall((stream) => { stream.respondWithFile(fname, { [HTTP2_HEADER_CONTENT_TYPE]: 'text/plain' }, { @@ -28,9 +28,9 @@ server.on('stream', (stream) => { headers[HTTP2_HEADER_CONTENT_LENGTH] = stat.size; } }); -}); -server.listen(0, () => { +})); +server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); @@ -49,4 +49,4 @@ server.listen(0, () => { server.close(); })); req.end(); -}); +})); From 3a34ebd81d4cd03284cba5c055cfa1b311504479 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 31 May 2018 10:39:19 +0200 Subject: [PATCH 32/67] http2: fix premature destroy Check stream._writableState.finished instead of stream.writable as the latter can lead to premature calls to destroy and dropped writes on busy processes. PR-URL: https://github.com/nodejs/node/pull/21051 Fixes: https://github.com/nodejs/node/issues/20750 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- lib/internal/http2/core.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 39430edee6ff4f..8f6c0c20b5b36c 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1892,7 +1892,7 @@ class Http2Stream extends Duplex { } // TODO(mcollina): remove usage of _*State properties - if (!this.writable) { + if (this._writableState.finished) { if (!this.readable && this.closed) { this.destroy(); return; From 3d38b99519d6ca37582298103c6f3a0e80c6b05f Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Tue, 5 Jun 2018 20:34:47 -0400 Subject: [PATCH 33/67] http2: safer Http2Session destructor It's hypothetically (and with certain V8 flags) possible for the session to be garbage collected before all the streams are. In that case, trying to remove the stream from the session will lead to a segfault due to attempting to access no longer valid memory. Fix this by unsetting the session on any streams still around when destroying. PR-URL: https://github.com/nodejs/node/pull/21194 Reviewed-By: Anna Henningsen Reviewed-By: Ujjwal Sharma --- src/node_http2.cc | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 4eb35330fd42e9..139d622e36781b 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -548,8 +548,8 @@ Http2Session::~Http2Session() { ClearWrap(object()); persistent().Reset(); CHECK(persistent().IsEmpty()); - for (const auto& iter : streams_) - iter.second->session_ = nullptr; + for (const auto& stream : streams_) + stream.second->session_ = nullptr; Unconsume(); DEBUG_HTTP2SESSION(this, "freeing nghttp2 session"); nghttp2_session_del(session_); @@ -1782,11 +1782,11 @@ Http2Stream::Http2Stream( Http2Stream::~Http2Stream() { - if (session_ != nullptr) { - DEBUG_HTTP2STREAM(this, "tearing down stream"); - session_->RemoveStream(this); - session_ = nullptr; - } + if (session_ == nullptr) + return; + DEBUG_HTTP2STREAM(this, "tearing down stream"); + session_->RemoveStream(this); + session_ = nullptr; persistent().Reset(); CHECK(persistent().IsEmpty()); @@ -1862,7 +1862,8 @@ inline void Http2Stream::Destroy() { // We can destroy the stream now if there are no writes for it // already on the socket. Otherwise, we'll wait for the garbage collector // to take care of cleaning up. - if (!stream->session()->HasWritesOnSocketForStream(stream)) + if (stream->session() == nullptr || + !stream->session()->HasWritesOnSocketForStream(stream)) delete stream; }, this, this->object()); From 4d46e3558d9739def5ffbeac31e8c30d36e9d540 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 15 Jun 2018 02:15:04 +0200 Subject: [PATCH 34/67] http2: fix memory leak for uncommon headers Fix a memory leak that occurs with header names that are short and not present in the static table of default headers. PR-URL: https://github.com/nodejs/node/pull/21336 Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell Reviewed-By: Minwoo Jung Reviewed-By: Richard Lau Reviewed-By: Tiancheng "Timothy" Gu --- src/node_http2.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/node_http2.h b/src/node_http2.h index f054ff9e9d6a26..67af832eff78b7 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -1299,6 +1299,7 @@ class ExternalHeader : } if (may_internalize && vec.len < 64) { + nghttp2_rcbuf_decref(buf); // This is a short header name, so there is a good chance V8 already has // it internalized. return GetInternalizedString(env, vec); From 78c7fc477a04404c07ea9e97b90289a68295d998 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 16 Jun 2018 23:07:31 +0200 Subject: [PATCH 35/67] http2: fix memory leak when headers are not emitted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When headers are not emitted to JS, e.g. because of an error before that could happen, we currently still have the vector of previously received headers lying around, each one holding a reference count of 1. To fix the resulting memory leak, release them in the `Http2Stream` destructor. Also, clear the vector of headers once they have been emitted – there’s not need to keep it around, wasting memory. PR-URL: https://github.com/nodejs/node/pull/21373 Reviewed-By: Tiancheng "Timothy" Gu --- src/node_http2.cc | 13 ++++++++----- src/node_http2.h | 8 ++------ 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 139d622e36781b..e41cd3cf0315c1 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1137,8 +1137,7 @@ inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { if (stream->IsDestroyed()) return; - nghttp2_header* headers = stream->headers(); - size_t count = stream->headers_count(); + std::vector headers(stream->move_headers()); Local name_str; Local value_str; @@ -1155,9 +1154,9 @@ inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { // this way for performance reasons (it's faster to generate and pass an // array than it is to generate and pass the object). size_t n = 0; - while (count > 0) { + while (n < headers.size()) { size_t j = 0; - while (count > 0 && j < arraysize(argv) / 2) { + while (n < headers.size() && j < arraysize(argv) / 2) { nghttp2_header item = headers[n++]; // The header name and value are passed as external one-byte strings name_str = @@ -1166,7 +1165,6 @@ inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { ExternalHeader::New(env(), item.value).ToLocalChecked(); argv[j * 2] = name_str; argv[j * 2 + 1] = value_str; - count--; j++; } // For performance, we pass name and value pairs to array.protototype.push @@ -1782,6 +1780,11 @@ Http2Stream::Http2Stream( Http2Stream::~Http2Stream() { + for (nghttp2_header& header : current_headers_) { + nghttp2_rcbuf_decref(header.name); + nghttp2_rcbuf_decref(header.value); + } + if (session_ == nullptr) return; DEBUG_HTTP2STREAM(this, "tearing down stream"); diff --git a/src/node_http2.h b/src/node_http2.h index 67af832eff78b7..a35dbf468afa43 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -653,18 +653,14 @@ class Http2Stream : public AsyncWrap, nghttp2_rcbuf* value, uint8_t flags); - inline nghttp2_header* headers() { - return current_headers_.data(); + inline std::vector move_headers() { + return std::move(current_headers_); } inline nghttp2_headers_category headers_category() const { return current_headers_category_; } - inline size_t headers_count() const { - return current_headers_.size(); - } - void StartHeaders(nghttp2_headers_category category); // Required for StreamBase From f8ca56405e939bd9cacfc2227cfd9fa213b74880 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 15 Jun 2018 00:52:35 +0200 Subject: [PATCH 36/67] http2: track memory allocated by nghttp2 Provide a custom memory allocator for nghttp2, and track memory allocated by the library with it. This makes the used-memory-per-session estimate more accurate, and allows us to track memory leaks either in nghttp2 itself or, more likely, through faulty usage on our end. It also allows us to make the per-session memory limit more accurate in the future; currently, we are not handling this in an ideal way, and instead let nghttp2 allocate what it wants, even if that goes over our limit. PR-URL: https://github.com/nodejs/node/pull/21374 Refs: https://github.com/nodejs/node/pull/21373 Refs: https://github.com/nodejs/node/pull/21336 Reviewed-By: James M Snell Reviewed-By: Anatoli Papirovski Reviewed-By: Tiancheng "Timothy" Gu --- src/node_http2.cc | 99 ++++++++++++++++++++++++++++++++++++++++++++--- src/node_http2.h | 19 ++++++--- src/util-inl.h | 5 ++- src/util.h | 3 ++ 4 files changed, 113 insertions(+), 13 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index e41cd3cf0315c1..b13632aa32d149 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -486,6 +486,92 @@ Http2Session::Callbacks::~Callbacks() { nghttp2_session_callbacks_del(callbacks); } +// Track memory allocated by nghttp2 using a custom allocator. +class Http2Session::MemoryAllocatorInfo { + public: + explicit MemoryAllocatorInfo(Http2Session* session) + : info({ session, H2Malloc, H2Free, H2Calloc, H2Realloc }) {} + + static void* H2Malloc(size_t size, void* user_data) { + return H2Realloc(nullptr, size, user_data); + } + + static void* H2Calloc(size_t nmemb, size_t size, void* user_data) { + size_t real_size = MultiplyWithOverflowCheck(nmemb, size); + void* mem = H2Malloc(real_size, user_data); + if (mem != nullptr) + memset(mem, 0, real_size); + return mem; + } + + static void H2Free(void* ptr, void* user_data) { + if (ptr == nullptr) return; // free(null); happens quite often. + void* result = H2Realloc(ptr, 0, user_data); + CHECK_EQ(result, nullptr); + } + + static void* H2Realloc(void* ptr, size_t size, void* user_data) { + Http2Session* session = static_cast(user_data); + size_t previous_size = 0; + char* original_ptr = nullptr; + + // We prepend each allocated buffer with a size_t containing the full + // size of the allocation. + if (size > 0) size += sizeof(size_t); + + if (ptr != nullptr) { + // We are free()ing or re-allocating. + original_ptr = static_cast(ptr) - sizeof(size_t); + previous_size = *reinterpret_cast(original_ptr); + // This means we called StopTracking() on this pointer before. + if (previous_size == 0) { + // Fall back to the standard Realloc() function. + char* ret = UncheckedRealloc(original_ptr, size); + if (ret != nullptr) + ret += sizeof(size_t); + return ret; + } + } + CHECK_GE(session->current_nghttp2_memory_, previous_size); + + // TODO(addaleax): Add the following, and handle NGHTTP2_ERR_NOMEM properly + // everywhere: + // + // if (size > previous_size && + // !session->IsAvailableSessionMemory(size - previous_size)) { + // return nullptr; + //} + + char* mem = UncheckedRealloc(original_ptr, size); + + if (mem != nullptr) { + // Adjust the memory info counter. + session->current_nghttp2_memory_ += size - previous_size; + *reinterpret_cast(mem) = size; + mem += sizeof(size_t); + } else if (size == 0) { + session->current_nghttp2_memory_ -= previous_size; + } + + return mem; + } + + static void StopTracking(Http2Session* session, void* ptr) { + size_t* original_ptr = reinterpret_cast( + static_cast(ptr) - sizeof(size_t)); + session->current_nghttp2_memory_ -= *original_ptr; + *original_ptr = 0; + } + + inline nghttp2_mem* operator*() { return &info; } + + nghttp2_mem info; +}; + +void Http2Session::StopTrackingRcbuf(nghttp2_rcbuf* buf) { + MemoryAllocatorInfo::StopTracking(this, buf); +} + Http2Session::Http2Session(Environment* env, Local wrap, nghttp2_session_type type) @@ -517,15 +603,17 @@ Http2Session::Http2Session(Environment* env, = callback_struct_saved[hasGetPaddingCallback ? 1 : 0].callbacks; auto fn = type == NGHTTP2_SESSION_SERVER ? - nghttp2_session_server_new2 : - nghttp2_session_client_new2; + nghttp2_session_server_new3 : + nghttp2_session_client_new3; + + MemoryAllocatorInfo allocator_info(this); // This should fail only if the system is out of memory, which // is going to cause lots of other problems anyway, or if any // of the options are out of acceptable range, which we should // be catching before it gets this far. Either way, crash if this // fails. - CHECK_EQ(fn(&session_, callbacks, this, *opts), 0); + CHECK_EQ(fn(&session_, callbacks, this, *opts, *allocator_info), 0); outgoing_storage_.reserve(4096); outgoing_buffers_.reserve(32); @@ -553,6 +641,7 @@ Http2Session::~Http2Session() { Unconsume(); DEBUG_HTTP2SESSION(this, "freeing nghttp2 session"); nghttp2_session_del(session_); + CHECK_EQ(current_nghttp2_memory_, 0); } inline bool HasHttp2Observer(Environment* env) { @@ -1160,9 +1249,9 @@ inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { nghttp2_header item = headers[n++]; // The header name and value are passed as external one-byte strings name_str = - ExternalHeader::New(env(), item.name).ToLocalChecked(); + ExternalHeader::New(this, item.name).ToLocalChecked(); value_str = - ExternalHeader::New(env(), item.value).ToLocalChecked(); + ExternalHeader::New(this, item.value).ToLocalChecked(); argv[j * 2] = name_str; argv[j * 2 + 1] = value_str; j++; diff --git a/src/node_http2.h b/src/node_http2.h index a35dbf468afa43..09771862f53bae 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -797,6 +797,7 @@ class Http2Session : public AsyncWrap { class Http2Ping; class Http2Settings; + class MemoryAllocatorInfo; inline void EmitStatistics(); @@ -934,13 +935,15 @@ class Http2Session : public AsyncWrap { current_session_memory_ -= amount; } - // Returns the current session memory including the current size of both - // the inflate and deflate hpack headers, the current outbound storage - // queue, and pending writes. + // Tell our custom memory allocator that this rcbuf is independent of + // this session now, and may outlive it. + void StopTrackingRcbuf(nghttp2_rcbuf* buf); + + // Returns the current session memory including memory allocated by nghttp2, + // the current outbound storage queue, and pending writes. uint64_t GetCurrentSessionMemory() { uint64_t total = current_session_memory_ + sizeof(Http2Session); - total += nghttp2_session_get_hd_deflate_dynamic_table_size(session_); - total += nghttp2_session_get_hd_inflate_dynamic_table_size(session_); + total += current_nghttp2_memory_; total += outgoing_storage_.size(); return total; } @@ -1072,6 +1075,8 @@ class Http2Session : public AsyncWrap { // The maximum amount of memory allocated for this session uint64_t max_session_memory_ = DEFAULT_MAX_SESSION_MEMORY; uint64_t current_session_memory_ = 0; + // The amount of memory allocated by nghttp2 internals + uint64_t current_nghttp2_memory_ = 0; // The collection of active Http2Streams associated with this session std::unordered_map streams_; @@ -1274,7 +1279,8 @@ class ExternalHeader : } template - static MaybeLocal New(Environment* env, nghttp2_rcbuf* buf) { + static MaybeLocal New(Http2Session* session, nghttp2_rcbuf* buf) { + Environment* env = session->env(); if (nghttp2_rcbuf_is_static(buf)) { auto& static_str_map = env->isolate_data()->http2_static_strs; v8::Eternal& eternal = static_str_map[buf]; @@ -1301,6 +1307,7 @@ class ExternalHeader : return GetInternalizedString(env, vec); } + session->StopTrackingRcbuf(buf); ExternalHeader* h_str = new ExternalHeader(buf); MaybeLocal str = String::NewExternalOneByte(env->isolate(), h_str); if (str.IsEmpty()) diff --git a/src/util-inl.h b/src/util-inl.h index 56c94148d6e820..b24baece7a5561 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -345,8 +345,9 @@ bool StringEqualNoCaseN(const char* a, const char* b, size_t length) { return true; } -inline size_t MultiplyWithOverflowCheck(size_t a, size_t b) { - size_t ret = a * b; +template +inline T MultiplyWithOverflowCheck(T a, T b) { + auto ret = a * b; if (a != 0) CHECK_EQ(b, ret / a); diff --git a/src/util.h b/src/util.h index 1cf515bb40fe3f..bf5e515083d447 100644 --- a/src/util.h +++ b/src/util.h @@ -65,6 +65,9 @@ inline char* Calloc(size_t n); inline char* UncheckedMalloc(size_t n); inline char* UncheckedCalloc(size_t n); +template +inline T MultiplyWithOverflowCheck(T a, T b); + // Used by the allocation functions when allocation fails. // Thin wrapper around v8::Isolate::LowMemoryNotification() that checks // whether V8 is initialized. From 9e64f917f20a60a449887b1b5c88daa375d288a3 Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich Date: Wed, 13 Jun 2018 00:26:42 +0200 Subject: [PATCH 37/67] doc: Improve doc for Http2 headers object Add more details regarding processing and data type of incoming headers in Http2. PR-URL: https://github.com/nodejs/node/pull/21296 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Ujjwal Sharma --- doc/api/http2.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/doc/api/http2.md b/doc/api/http2.md index 3ffb41e1402ca2..c5c4cb4c95d352 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -2120,6 +2120,23 @@ prototype. This means that normal JavaScript object methods such as `Object.prototype.toString()` and `Object.prototype.hasOwnProperty()` will not work. +For incoming headers: +* The `:status` header is converted to `number`. +* Duplicates of `:status`, `:method`, `:authority`, `:scheme`, `:path`, +`age`, `authorization`, `access-control-allow-credentials`, +`access-control-max-age`, `access-control-request-method`, `content-encoding`, +`content-language`, `content-length`, `content-location`, `content-md5`, +`content-range`, `content-type`, `date`, `dnt`, `etag`, `expires`, `from`, +`if-match`, `if-modified-since`, `if-none-match`, `if-range`, +`if-unmodified-since`, `last-modified`, `location`, `max-forwards`, +`proxy-authorization`, `range`, `referer`,`retry-after`, `tk`, +`upgrade-insecure-requests`, `user-agent` or `x-content-type-options` are +discarded. +* `set-cookie` is a string if present once or an array in case duplicates +are present. +* `cookie`: the values are joined together with '; '. +* For all other headers, the values are joined together with ', '. + ```js const http2 = require('http2'); const server = http2.createServer(); From 6bdee91156544c5c33c3e732c34cb8ce65040b00 Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich Date: Fri, 15 Jun 2018 23:37:46 +0200 Subject: [PATCH 38/67] http2: pass incoming set-cookie header as array Incoming set-cookie headers should be passed to user as array like in http module. Besides improving compatibility between http and http2 it avoids the need to check if the type is an array or not in user code. PR-URL: https://github.com/nodejs/node/pull/21360 Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Matteo Collina --- doc/api/http2.md | 3 +- lib/internal/http2/util.js | 7 +-- test/parallel/test-http2-util-headers-list.js | 44 +++++++++++++++++-- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index c5c4cb4c95d352..f180a7d297c372 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -2132,8 +2132,7 @@ For incoming headers: `proxy-authorization`, `range`, `referer`,`retry-after`, `tk`, `upgrade-insecure-requests`, `user-agent` or `x-content-type-options` are discarded. -* `set-cookie` is a string if present once or an array in case duplicates -are present. +* `set-cookie` is always an array. Duplicates are added to the array. * `cookie`: the values are joined together with '; '. * For all other headers, the values are joined together with ', '. diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index ef48b83d783af0..e7ef7db59077b3 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -504,7 +504,7 @@ function toHeaderObject(headers) { value |= 0; var existing = obj[name]; if (existing === undefined) { - obj[name] = value; + obj[name] = name === HTTP2_HEADER_SET_COOKIE ? [value] : value; } else if (!kSingleValueHeaders.has(name)) { switch (name) { case HTTP2_HEADER_COOKIE: @@ -523,10 +523,7 @@ function toHeaderObject(headers) { // fields with the same name. Since it cannot be combined into a // single field-value, recipients ought to handle "Set-Cookie" as a // special case while processing header fields." - if (Array.isArray(existing)) - existing.push(value); - else - obj[name] = [existing, value]; + existing.push(value); break; default: // https://tools.ietf.org/html/rfc7230#section-3.2.2 diff --git a/test/parallel/test-http2-util-headers-list.js b/test/parallel/test-http2-util-headers-list.js index 0ff6b558d9a51b..3b594e727cc5ef 100644 --- a/test/parallel/test-http2-util-headers-list.js +++ b/test/parallel/test-http2-util-headers-list.js @@ -1,14 +1,14 @@ // Flags: --expose-internals 'use strict'; -// Tests the internal utility function that is used to prepare headers -// to pass to the internal binding layer. +// Tests the internal utility functions that are used to prepare headers +// to pass to the internal binding layer and to build a header object. const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); -const { mapToHeaders } = require('internal/http2/util'); +const { mapToHeaders, toHeaderObject } = require('internal/http2/util'); const { HTTP2_HEADER_STATUS, @@ -302,3 +302,41 @@ common.expectsError({ assert(!(mapToHeaders({ te: 'trailers' }) instanceof Error)); assert(!(mapToHeaders({ te: ['trailers'] }) instanceof Error)); + + +{ + const rawHeaders = [ + ':status', '200', + 'cookie', 'foo', + 'set-cookie', 'sc1', + 'age', '10', + 'x-multi', 'first' + ]; + const headers = toHeaderObject(rawHeaders); + assert.strictEqual(headers[':status'], 200); + assert.strictEqual(headers.cookie, 'foo'); + assert.deepStrictEqual(headers['set-cookie'], ['sc1']); + assert.strictEqual(headers.age, '10'); + assert.strictEqual(headers['x-multi'], 'first'); +} + +{ + const rawHeaders = [ + ':status', '200', + ':status', '400', + 'cookie', 'foo', + 'cookie', 'bar', + 'set-cookie', 'sc1', + 'set-cookie', 'sc2', + 'age', '10', + 'age', '20', + 'x-multi', 'first', + 'x-multi', 'second' + ]; + const headers = toHeaderObject(rawHeaders); + assert.strictEqual(headers[':status'], 200); + assert.strictEqual(headers.cookie, 'foo; bar'); + assert.deepStrictEqual(headers['set-cookie'], ['sc1', 'sc2']); + assert.strictEqual(headers.age, '10'); + assert.strictEqual(headers['x-multi'], 'first, second'); +} From 00002a659dc6031b4a00f69373a116bccb40dc8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=A1=D0=BA=D0=BE=D0=B2=D0=BE=D1=80=D0=BE=D0=B4=D0=B0=20?= =?UTF-8?q?=D0=9D=D0=B8=D0=BA=D0=B8=D1=82=D0=B0=20=D0=90=D0=BD=D0=B4=D1=80?= =?UTF-8?q?=D0=B5=D0=B5=D0=B2=D0=B8=D1=87?= Date: Sat, 23 Jun 2018 14:48:54 +0300 Subject: [PATCH 39/67] doc: fix http2stream.pushStream error doc The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit 0babd181a0c5d775e62a12b3b04fe4d7654fe80a (pull request #17406), and the testcase for http2stream.pushStream was changed accordingly, but the documentation change was overlooked. This commit fixes it and aligns the documentation with the testcase. This is a part of the fixes hinted by #21470, which includes some tests for error codes usage and documentation and enforces a stricter format. Refs: https://github.com/nodejs/node/pull/21470 Refs: https://github.com/nodejs/node/pull/17406 PR-URL: https://github.com/nodejs/node/pull/21487 Reviewed-By: Colin Ihrig Reviewed-By: Trivikram Kamat Reviewed-By: Vse Mozhet Byt Reviewed-By: Luigi Pinca --- doc/api/http2.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index f180a7d297c372..28e915519258f1 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -3145,7 +3145,7 @@ added: v8.4.0 Call [`http2stream.pushStream()`][] with the given headers, and wraps the given newly created [`Http2Stream`] on `Http2ServerResponse`. -The callback will be called with an error with code `ERR_HTTP2_STREAM_CLOSED` +The callback will be called with an error with code `ERR_HTTP2_INVALID_STREAM` if the stream is closed. ## Collecting HTTP/2 Performance Metrics From 823b31e06e2997acece8c9c81d6822d25c90218a Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Fri, 6 Jul 2018 09:48:59 -0700 Subject: [PATCH 40/67] http2: order declarations in core.js Order declarations: * public modules in alphabetical order * internal modules in alphabetical order * process.binding() calls in alphabetical order * exports in alphabetical order PR-URL: https://github.com/nodejs/node/pull/21689 Reviewed-By: Gus Caplan Reviewed-By: Weijia Wang Reviewed-By: James M Snell --- lib/internal/http2/core.js | 70 +++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 32 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 8f6c0c20b5b36c..9beb8833a297fb 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2,42 +2,38 @@ /* eslint-disable no-use-before-define */ -require('internal/util').assertCrypto(); +const { + assertCrypto, + customInspectSymbol: kInspect, + promisify +} = require('internal/util'); + +assertCrypto(); -const { async_id_symbol } = process.binding('async_wrap'); -const http = require('http'); -const binding = process.binding('http2'); const assert = require('assert'); const { Buffer } = require('buffer'); const EventEmitter = require('events'); +const fs = require('fs'); +const http = require('http'); const net = require('net'); +const { Duplex } = require('stream'); +const { + _unrefActive, + enroll, + unenroll +} = require('timers'); const tls = require('tls'); +const { URL } = require('url'); const util = require('util'); -const fs = require('fs'); -const errors = require('internal/errors'); + const { StreamWrap } = require('_stream_wrap'); -const { Duplex } = require('stream'); -const { URL } = require('url'); + +const errors = require('internal/errors'); +const { utcDate } = require('internal/http'); const { onServerStream, Http2ServerRequest, Http2ServerResponse, } = require('internal/http2/compat'); -const { utcDate } = require('internal/http'); -const { - promisify, - customInspectSymbol: kInspect -} = require('internal/util'); -const { isArrayBufferView } = require('internal/util/types'); -const { _connectionListener: httpConnectionListener } = http; -const { createPromise, promiseResolve } = process.binding('util'); -const debug = util.debuglog('http2'); - -const kMaxFrameSize = (2 ** 24) - 1; -const kMaxInt = (2 ** 32) - 1; -const kMaxStreams = (2 ** 31) - 1; - -// eslint-disable-next-line no-control-regex -const kQuotedString = /^[\x09\x20-\x5b\x5d-\x7e\x80-\xff]*$/; const { assertIsObject, @@ -58,13 +54,23 @@ const { updateSettingsBuffer } = require('internal/http2/util'); -const { - _unrefActive, - enroll, - unenroll -} = require('timers'); +const { isArrayBufferView } = require('internal/util/types'); +const { async_id_symbol } = process.binding('async_wrap'); +const binding = process.binding('http2'); const { ShutdownWrap, WriteWrap } = process.binding('stream_wrap'); +const { createPromise, promiseResolve } = process.binding('util'); + +const { _connectionListener: httpConnectionListener } = http; +const debug = util.debuglog('http2'); + +const kMaxFrameSize = (2 ** 24) - 1; +const kMaxInt = (2 ** 32) - 1; +const kMaxStreams = (2 ** 31) - 1; + +// eslint-disable-next-line no-control-regex +const kQuotedString = /^[\x09\x20-\x5b\x5d-\x7e\x80-\xff]*$/; + const { constants, nameForErrorCode } = binding; const NETServer = net.Server; @@ -2784,13 +2790,13 @@ function getUnpackedSettings(buf, options = {}) { // Exports module.exports = { + connect, constants, + createServer, + createSecureServer, getDefaultSettings, getPackedSettings, getUnpackedSettings, - createServer, - createSecureServer, - connect, Http2Session, Http2Stream, Http2ServerRequest, From a9053faa8bb0c5185dc2bb5ec53f19be86675391 Mon Sep 17 00:00:00 2001 From: Shailesh Shekhawat Date: Wed, 27 Jun 2018 22:05:32 +0800 Subject: [PATCH 41/67] errors: fix undefined HTTP2 and tls errors Includes implementation of tls, HTTP2 error with documentation. PR-URL: https://github.com/nodejs/node/pull/21564 Refs: https://github.com/nodejs/node/issues/21440 Reviewed-By: Matteo Collina Reviewed-By: Anatoli Papirovski --- doc/api/errors.md | 5 +++++ lib/internal/errors.js | 1 + 2 files changed, 6 insertions(+) diff --git a/doc/api/errors.md b/doc/api/errors.md index 1ab394580e7b76..13193f95ca9af8 100755 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -826,6 +826,11 @@ send something other than a regular file. The `Http2Session` closed with a non-zero error code. + +### ERR_HTTP2_SETTINGS_CANCEL + +The `Http2Session` settings canceled. + ### ERR_HTTP2_SOCKET_BOUND diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 7a26d863fe0818..d216e9c37cf0b0 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -333,6 +333,7 @@ E('ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED', 'Cannot set HTTP/2 pseudo-headers'); E('ERR_HTTP2_PUSH_DISABLED', 'HTTP/2 client has disabled push streams'); E('ERR_HTTP2_SEND_FILE', 'Only regular files can be sent'); E('ERR_HTTP2_SESSION_ERROR', 'Session closed with error code %s'); +E('ERR_HTTP2_SETTINGS_CANCEL', 'HTTP2 session settings canceled'); E('ERR_HTTP2_SOCKET_BOUND', 'The socket is already bound to an Http2Session'); E('ERR_HTTP2_STATUS_101', From 9a0d117a2cbda6544b3dd17d616315fbe5823c6d Mon Sep 17 00:00:00 2001 From: RidgeA Date: Wed, 11 Jul 2018 17:29:42 +0300 Subject: [PATCH 42/67] http2: remove `waitTrailers` listener after closing a stream When `writeHeader` of `Http2ServerResponse` instance are called with 204, 205 and 304 status codes an underlying stream closes. If call `end` method after sending any of these status codes it will cause an error `TypeError: Cannot read property 'Symbol(trailers)' of undefined` because a reference to `Http2ServerResponse` instance associated with Http2Stream already was deleted. The closing of stream causes emitting `waitTrailers` event and, when this event handles inside `onStreamTrailerReady` handler, there is no reference to Http2ServerResponse instance. Fixes: https://github.com/nodejs/node/issues/21740 PR-URL: https://github.com/nodejs/node/pull/21764 Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Ruben Bridgewater Reviewed-By: Anatoli Papirovski --- lib/internal/http2/compat.js | 2 + ...esponse-end-after-statuses-without-body.js | 47 +++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 test/parallel/test-http2-compat-serverresponse-end-after-statuses-without-body.js diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 9b2367c4c97a57..d0cfe94d48a281 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -377,6 +377,8 @@ function onStreamCloseResponse() { state.closed = true; this[kProxySocket] = null; + + this.removeListener('wantTrailers', onStreamTrailersReady); this[kResponse] = undefined; res.emit('finish'); diff --git a/test/parallel/test-http2-compat-serverresponse-end-after-statuses-without-body.js b/test/parallel/test-http2-compat-serverresponse-end-after-statuses-without-body.js new file mode 100644 index 00000000000000..83d5521bf2473f --- /dev/null +++ b/test/parallel/test-http2-compat-serverresponse-end-after-statuses-without-body.js @@ -0,0 +1,47 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const h2 = require('http2'); + +// This test case ensures that calling of res.end after sending +// 204, 205 and 304 HTTP statuses will not cause an error +// See issue: https://github.com/nodejs/node/issues/21740 + +const { + HTTP_STATUS_NO_CONTENT, + HTTP_STATUS_RESET_CONTENT, + HTTP_STATUS_NOT_MODIFIED +} = h2.constants; + +const statusWithouBody = [ + HTTP_STATUS_NO_CONTENT, + HTTP_STATUS_RESET_CONTENT, + HTTP_STATUS_NOT_MODIFIED, +]; +const STATUS_CODES_COUNT = statusWithouBody.length; + +const server = h2.createServer(common.mustCall(function(req, res) { + res.writeHead(statusWithouBody.pop()); + res.end(); +}, STATUS_CODES_COUNT)); + +server.listen(0, common.mustCall(function() { + const url = `http://localhost:${server.address().port}`; + const client = h2.connect(url, common.mustCall(() => { + let responseCount = 0; + const closeAfterResponse = () => { + if (STATUS_CODES_COUNT === ++responseCount) { + client.destroy(); + server.close(); + } + }; + + for (let i = 0; i < STATUS_CODES_COUNT; i++) { + const request = client.request(); + request.on('response', common.mustCall(closeAfterResponse)); + } + + })); +})); From 41edcf4ab1b7e8772ea8bf8ae0eaa487d33be9c0 Mon Sep 17 00:00:00 2001 From: Kevin Simper Date: Tue, 17 Jul 2018 15:26:04 -0700 Subject: [PATCH 43/67] doc: add missing `require` to example in http2.md PR-URL: https://github.com/nodejs/node/pull/21858 Reviewed-By: Gus Caplan Reviewed-By: Rich Trott Reviewed-By: Daijiro Wachi Reviewed-By: James M Snell Reviewed-By: Vse Mozhet Byt Reviewed-By: Trivikram Kamat --- doc/api/http2.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/api/http2.md b/doc/api/http2.md index 28e915519258f1..dd201c9dd7dec2 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -1921,6 +1921,7 @@ instances. ```js const http2 = require('http2'); +const fs = require('fs'); const options = { key: fs.readFileSync('server-key.pem'), From d78a7b355baad1e48102534adc8486c8c61482b6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 15 Jul 2018 23:58:13 +0200 Subject: [PATCH 44/67] http2: remove unused nghttp2 error list Remove a list of HTTP2 errors as well as `nghttp2_errname()` that converted an integer nghttp2 error code to a string representation. We already use `nghttp2_strerror()` for this, which is provided by nghttp2 returns a better error string anyway. PR-URL: https://github.com/nodejs/node/pull/21827 Reviewed-By: Colin Ihrig Reviewed-By: Trivikram Kamat Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Luigi Pinca --- src/node_http2.h | 52 ------------------------------------------------ 1 file changed, 52 deletions(-) diff --git a/src/node_http2.h b/src/node_http2.h index 09771862f53bae..219d1d02527088 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -377,58 +377,6 @@ enum padding_strategy_type { PADDING_STRATEGY_CALLBACK }; -// These are the error codes provided by the underlying nghttp2 implementation. -#define NGHTTP2_ERROR_CODES(V) \ - V(NGHTTP2_ERR_INVALID_ARGUMENT) \ - V(NGHTTP2_ERR_BUFFER_ERROR) \ - V(NGHTTP2_ERR_UNSUPPORTED_VERSION) \ - V(NGHTTP2_ERR_WOULDBLOCK) \ - V(NGHTTP2_ERR_PROTO) \ - V(NGHTTP2_ERR_INVALID_FRAME) \ - V(NGHTTP2_ERR_EOF) \ - V(NGHTTP2_ERR_DEFERRED) \ - V(NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE) \ - V(NGHTTP2_ERR_STREAM_CLOSED) \ - V(NGHTTP2_ERR_STREAM_CLOSING) \ - V(NGHTTP2_ERR_STREAM_SHUT_WR) \ - V(NGHTTP2_ERR_INVALID_STREAM_ID) \ - V(NGHTTP2_ERR_INVALID_STREAM_STATE) \ - V(NGHTTP2_ERR_DEFERRED_DATA_EXIST) \ - V(NGHTTP2_ERR_START_STREAM_NOT_ALLOWED) \ - V(NGHTTP2_ERR_GOAWAY_ALREADY_SENT) \ - V(NGHTTP2_ERR_INVALID_HEADER_BLOCK) \ - V(NGHTTP2_ERR_INVALID_STATE) \ - V(NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) \ - V(NGHTTP2_ERR_FRAME_SIZE_ERROR) \ - V(NGHTTP2_ERR_HEADER_COMP) \ - V(NGHTTP2_ERR_FLOW_CONTROL) \ - V(NGHTTP2_ERR_INSUFF_BUFSIZE) \ - V(NGHTTP2_ERR_PAUSE) \ - V(NGHTTP2_ERR_TOO_MANY_INFLIGHT_SETTINGS) \ - V(NGHTTP2_ERR_PUSH_DISABLED) \ - V(NGHTTP2_ERR_DATA_EXIST) \ - V(NGHTTP2_ERR_SESSION_CLOSING) \ - V(NGHTTP2_ERR_HTTP_HEADER) \ - V(NGHTTP2_ERR_HTTP_MESSAGING) \ - V(NGHTTP2_ERR_REFUSED_STREAM) \ - V(NGHTTP2_ERR_INTERNAL) \ - V(NGHTTP2_ERR_CANCEL) \ - V(NGHTTP2_ERR_FATAL) \ - V(NGHTTP2_ERR_NOMEM) \ - V(NGHTTP2_ERR_CALLBACK_FAILURE) \ - V(NGHTTP2_ERR_BAD_CLIENT_MAGIC) \ - V(NGHTTP2_ERR_FLOODED) - -const char* nghttp2_errname(int rv) { - switch (rv) { -#define V(code) case code: return #code; - NGHTTP2_ERROR_CODES(V) -#undef V - default: - return "NGHTTP2_UNKNOWN_ERROR"; - } -} - enum session_state_flags { SESSION_STATE_NONE = 0x0, SESSION_STATE_HAS_SCOPE = 0x1, From aa4c19b079ec315fc9cee82bb6c1df7739f6c0b9 Mon Sep 17 00:00:00 2001 From: James Ide Date: Fri, 20 Jul 2018 17:25:02 -0700 Subject: [PATCH 45/67] http2: release request()'s "connect" event listener after it runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `Http2Session#request()` method internally listens to the "connect" event if the session has not yet established a connection so that the actual request can be sent after the connection has been established. This commit removes the event listener after it runs and carries out the request and is no longer needed. In practice this shouldn't affect the behavior of the session object since the "connect" event fires only once anyway, but removing the listener releases its references. The rest of this class subscribes to the "connect" event with `once` instead of `on` as well. Tested by adding a new test that ensures `Http2Session#request()` is called before the connection is established, indicated by a "connect" listener that is run. The test also ensures all "connect" listeners are removed after the connection is established. PR-URL: https://github.com/nodejs/node/pull/21916 Reviewed-By: Michaël Zasso Reviewed-By: Trivikram Kamat Reviewed-By: Anna Henningsen Reviewed-By: Anatoli Papirovski Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- lib/internal/http2/core.js | 2 +- ...t-http2-request-remove-connect-listener.js | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-http2-request-remove-connect-listener.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 9beb8833a297fb..e496c3a87fd3ee 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1384,7 +1384,7 @@ class ClientHttp2Session extends Http2Session { const onConnect = requestOnConnect.bind(stream, headersList, options); if (this.connecting) { - this.on('connect', onConnect); + this.once('connect', onConnect); } else { onConnect(); } diff --git a/test/parallel/test-http2-request-remove-connect-listener.js b/test/parallel/test-http2-request-remove-connect-listener.js new file mode 100644 index 00000000000000..61de140c225144 --- /dev/null +++ b/test/parallel/test-http2-request-remove-connect-listener.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); + + +const server = http2.createServer(); +server.on('stream', common.mustCall((stream) => { + stream.respond(); + stream.end(); +})); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request(); + client.once('connect', common.mustCall()); + + req.on('response', common.mustCall(() => { + assert.strictEqual(client.listenerCount('connect'), 0); + })); + req.on('close', common.mustCall(() => { + server.close(); + client.close(); + })); +})); From 833ef1708684a679522788e19fdda076863f2518 Mon Sep 17 00:00:00 2001 From: Anto Aravinth Date: Sat, 28 Jul 2018 10:00:32 +0530 Subject: [PATCH 46/67] test: refactor test-http2-compat-serverresponse-finished.js PR-URL: https://github.com/nodejs/node/pull/21929 Reviewed-By: Luigi Pinca Reviewed-By: Jon Moss Reviewed-By: Trivikram Kamat Reviewed-By: Benjamin Gruenbaum Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- .../test-http2-compat-serverresponse-finished.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-http2-compat-serverresponse-finished.js b/test/parallel/test-http2-compat-serverresponse-finished.js index ceaa6eb5c3cf2c..4da592a5b354b0 100644 --- a/test/parallel/test-http2-compat-serverresponse-finished.js +++ b/test/parallel/test-http2-compat-serverresponse-finished.js @@ -9,14 +9,14 @@ const net = require('net'); // Http2ServerResponse.finished const server = h2.createServer(); -server.listen(0, common.mustCall(function() { +server.listen(0, common.mustCall(() => { const port = server.address().port; - server.once('request', common.mustCall(function(request, response) { + server.once('request', common.mustCall((request, response) => { assert.ok(response.socket instanceof net.Socket); assert.ok(response.connection instanceof net.Socket); assert.strictEqual(response.socket, response.connection); - response.on('finish', common.mustCall(function() { + response.on('finish', common.mustCall(() => { assert.strictEqual(response.socket, undefined); assert.strictEqual(response.connection, undefined); process.nextTick(common.mustCall(() => { @@ -30,7 +30,7 @@ server.listen(0, common.mustCall(function() { })); const url = `http://localhost:${port}`; - const client = h2.connect(url, common.mustCall(function() { + const client = h2.connect(url, common.mustCall(() => { const headers = { ':path': '/', ':method': 'GET', @@ -38,7 +38,7 @@ server.listen(0, common.mustCall(function() { ':authority': `localhost:${port}` }; const request = client.request(headers); - request.on('end', common.mustCall(function() { + request.on('end', common.mustCall(() => { client.close(); })); request.end(); From 23b53e706b79ae91659c1b9edf5a85f5e333ca56 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 29 Jul 2018 19:26:27 -0700 Subject: [PATCH 47/67] test: improve reliability in http2-session-timeout Use `setImmediate()` instead of `setTimeout()` to improve robustness of test-http2-session-timeout. Fixes: https://github.com/nodejs/node/issues/20628 PR-URL: https://github.com/nodejs/node/pull/22026 Reviewed-By: Anatoli Papirovski Reviewed-By: Jon Moss Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat --- test/sequential/test-http2-session-timeout.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/sequential/test-http2-session-timeout.js b/test/sequential/test-http2-session-timeout.js index 48e98998c700b6..5c4f047b338e9c 100644 --- a/test/sequential/test-http2-session-timeout.js +++ b/test/sequential/test-http2-session-timeout.js @@ -3,13 +3,12 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); -const h2 = require('http2'); +const http2 = require('http2'); const serverTimeout = common.platformTimeout(200); -const callTimeout = common.platformTimeout(20); const mustNotCall = common.mustNotCall(); -const server = h2.createServer(); +const server = http2.createServer(); server.timeout = serverTimeout; server.on('request', (req, res) => res.end()); @@ -19,7 +18,7 @@ server.listen(0, common.mustCall(() => { const port = server.address().port; const url = `http://localhost:${port}`; - const client = h2.connect(url); + const client = http2.connect(url); const startTime = process.hrtime(); makeReq(); @@ -37,7 +36,7 @@ server.listen(0, common.mustCall(() => { const diff = process.hrtime(startTime); const milliseconds = (diff[0] * 1e3 + diff[1] / 1e6); if (milliseconds < serverTimeout * 2) { - setTimeout(makeReq, callTimeout); + setImmediate(makeReq); } else { server.removeListener('timeout', mustNotCall); server.close(); From 07ef45defb12317e5626b6c546448b0d18f14024 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 10 Aug 2018 11:16:45 -0700 Subject: [PATCH 48/67] http2: remove `streamError` from docs `streamError` was removed quite some time ago but the docs and code comments weren't updated. Fix that. Fixes: https://github.com/nodejs/node/issues/20211 PR-URL: https://github.com/nodejs/node/pull/22246 Reviewed-By: Luigi Pinca Reviewed-By: Jon Moss Reviewed-By: Colin Ihrig Reviewed-By: Matteo Collina Reviewed-By: Vse Mozhet Byt Reviewed-By: Trivikram Kamat --- doc/api/http2.md | 12 ------------ lib/internal/http2/compat.js | 4 +--- .../test-http2-compat-serverresponse-destroy.js | 3 +-- 3 files changed, 2 insertions(+), 17 deletions(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index dd201c9dd7dec2..85b3ba4363ada5 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -1505,10 +1505,6 @@ added: v8.4.0 * Extends: {net.Server} -In `Http2Server`, there are no `'clientError'` events as there are in -HTTP1. However, there are `'sessionError'`, and `'streamError'` events for -errors emitted on the socket, or from `Http2Session` or `Http2Stream` instances. - #### Event: 'checkContinue' - -If a `ServerHttp2Stream` emits an `'error'` event, it will be forwarded here. -The stream will already be destroyed when this event is triggered. - #### Event: 'stream' +* `headers` {HTTP/2 Headers Object} An object describing the headers +* `callback` {Function} Called once `http2stream.pushStream()` is finished, + or either when the attempt to create the pushed `Http2Stream` has failed or + has been rejected, or the state of `Http2ServerRequest` is closed prior to + calling the `http2stream.pushStream()` method + * `err` {Error} + * `stream` {ServerHttp2Stream} The newly-created `ServerHttp2Stream` object -Call [`http2stream.pushStream()`][] with the given headers, and wraps the -given newly created [`Http2Stream`] on `Http2ServerResponse`. - -The callback will be called with an error with code `ERR_HTTP2_INVALID_STREAM` -if the stream is closed. +Call [`http2stream.pushStream()`][] with the given headers, and wrap the +given [`Http2Stream`] on a newly created `Http2ServerResponse` as the callback +parameter if successful. When `Http2ServerRequest` is closed, the callback is +called with an error `ERR_HTTP2_INVALID_STREAM`. ## Collecting HTTP/2 Performance Metrics From 391e30aeb3e09b96e94f908bb75f3aecc3651210 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 26 Aug 2018 15:45:53 -0700 Subject: [PATCH 55/67] doc: simplify http2 wording and formatting Remove `It is important to note that` and italics from `waitForTrailers` sentences. PR-URL: https://github.com/nodejs/node/pull/22541 Reviewed-By: Vse Mozhet Byt Reviewed-By: Luigi Pinca --- doc/api/http2.md | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index ea53e7c48d50b9..7a8abcf4b58498 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -744,10 +744,10 @@ 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. -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`. +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: @@ -1290,10 +1290,10 @@ will be emitted immediately after queuing the last chunk of payload data to be sent. The `http2stream.sendTrailers()` method can then be used to sent trailing header fields to the peer. -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`. +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`. ```js const http2 = require('http2'); @@ -1369,10 +1369,10 @@ will be emitted immediately after queuing the last chunk of payload data to be sent. The `http2stream.sendTrailers()` method can then be used to sent trailing header fields to the peer. -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`. +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`. ```js const http2 = require('http2'); @@ -1488,10 +1488,10 @@ will be emitted immediately after queuing the last chunk of payload data to be sent. The `http2stream.sendTrilers()` method can then be used to sent trailing header fields to the peer. -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`. +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`. ```js const http2 = require('http2'); From 36405688157a769fd4f1de3e4778c0baa8ee4a51 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 23 Aug 2018 10:38:31 -0700 Subject: [PATCH 56/67] http2: throw better error when accessing unbound socket proxy Fixes: https://github.com/nodejs/node/issues/22268 PR-URL: https://github.com/nodejs/node/pull/22486 Reviewed-By: Ruben Bridgewater Reviewed-By: Rich Trott Reviewed-By: Trivikram Kamat Reviewed-By: Matteo Collina --- doc/api/errors.md | 6 ++ lib/internal/errors.js | 2 + lib/internal/http2/core.js | 12 +++- .../test-http2-unbound-socket-proxy.js | 69 +++++++++++++++++++ 4 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-http2-unbound-socket-proxy.js diff --git a/doc/api/errors.md b/doc/api/errors.md index 8632bb7be28484..d7c5da0ceb4f58 100755 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -843,6 +843,12 @@ The `Http2Session` settings canceled. An attempt was made to connect a `Http2Session` object to a `net.Socket` or `tls.TLSSocket` that had already been bound to another `Http2Session` object. + +### ERR_HTTP2_SOCKET_UNBOUND + +An attempt was made to use the `socket` property of an `Http2Session` that +has already been closed. + ### ERR_HTTP2_STATUS_101 diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 587fce65613374..0863f951830938 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -338,6 +338,8 @@ E('ERR_HTTP2_SESSION_ERROR', 'Session closed with error code %s'); E('ERR_HTTP2_SETTINGS_CANCEL', 'HTTP2 session settings canceled'); E('ERR_HTTP2_SOCKET_BOUND', 'The socket is already bound to an Http2Session'); +E('ERR_HTTP2_SOCKET_UNBOUND', + 'The socket has been disconnected from the Http2Session'); E('ERR_HTTP2_STATUS_101', 'HTTP status code 101 (Switching Protocols) is forbidden in HTTP/2'); E('ERR_HTTP2_STATUS_INVALID', 'Invalid status code: %s'); diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 2dea50ea065556..9aa7a0e2a2f79e 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -641,12 +641,17 @@ const proxySocketHandler = { throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION'); default: const socket = session[kSocket]; + if (socket === undefined) + throw new errors.Error('ERR_HTTP2_SOCKET_UNBOUND'); const value = socket[prop]; return typeof value === 'function' ? value.bind(socket) : value; } }, getPrototypeOf(session) { - return Reflect.getPrototypeOf(session[kSocket]); + const socket = session[kSocket]; + if (socket === undefined) + throw new errors.Error('ERR_HTTP2_SOCKET_UNBOUND'); + return Reflect.getPrototypeOf(socket); }, set(session, prop, value) { switch (prop) { @@ -662,7 +667,10 @@ const proxySocketHandler = { case 'write': throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION'); default: - session[kSocket][prop] = value; + const socket = session[kSocket]; + if (socket === undefined) + throw new errors.Error('ERR_HTTP2_SOCKET_UNBOUND'); + socket[prop] = value; return true; } } diff --git a/test/parallel/test-http2-unbound-socket-proxy.js b/test/parallel/test-http2-unbound-socket-proxy.js new file mode 100644 index 00000000000000..44f113bac962f7 --- /dev/null +++ b/test/parallel/test-http2-unbound-socket-proxy.js @@ -0,0 +1,69 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const http2 = require('http2'); +const net = require('net'); + +const server = http2.createServer(); +server.on('stream', common.mustCall((stream) => { + stream.respond(); + stream.end('ok'); +})); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + const socket = client.socket; + const req = client.request(); + req.resume(); + req.on('close', common.mustCall(() => { + client.close(); + server.close(); + + // Tests to make sure accessing the socket proxy fails with an + // informative error. + setImmediate(common.mustCall(() => { + common.expectsError(() => { + socket.example; + }, { + code: 'ERR_HTTP2_SOCKET_UNBOUND' + }); + common.expectsError(() => { + socket.example = 1; + }, { + code: 'ERR_HTTP2_SOCKET_UNBOUND' + }); + common.expectsError(() => { + socket instanceof net.Socket; + }, { + code: 'ERR_HTTP2_SOCKET_UNBOUND' + }); + common.expectsError(() => { + socket.ref(); + }, { + code: 'ERR_HTTP2_SOCKET_UNBOUND' + }); + common.expectsError(() => { + socket.unref(); + }, { + code: 'ERR_HTTP2_SOCKET_UNBOUND' + }); + common.expectsError(() => { + socket.setEncoding(); + }, { + code: 'ERR_HTTP2_SOCKET_UNBOUND' + }); + common.expectsError(() => { + socket.setKeepAlive(); + }, { + code: 'ERR_HTTP2_SOCKET_UNBOUND' + }); + common.expectsError(() => { + socket.setNoDelay(); + }, { + code: 'ERR_HTTP2_SOCKET_UNBOUND' + }); + })); + })); +})); From f6d9bf3d9d874d4b05124e321b6e865234f79c22 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 2 Sep 2018 12:00:41 +0200 Subject: [PATCH 57/67] deps: update to nghttp2 1.33.0 Refs: https://github.com/nghttp2/nghttp2/releases/tag/v1.33.0 PR-URL: https://github.com/nodejs/node/pull/22649 Reviewed-By: Colin Ihrig Reviewed-By: Matteo Collina Reviewed-By: Trivikram Kamat --- deps/nghttp2/lib/includes/nghttp2/nghttp2.h | 142 +++++++++++---- .../nghttp2/lib/includes/nghttp2/nghttp2ver.h | 4 +- deps/nghttp2/lib/nghttp2_buf.h | 2 +- deps/nghttp2/lib/nghttp2_callbacks.h | 2 +- deps/nghttp2/lib/nghttp2_debug.h | 10 +- deps/nghttp2/lib/nghttp2_frame.c | 130 ++++++++++++++ deps/nghttp2/lib/nghttp2_frame.h | 51 +++++- deps/nghttp2/lib/nghttp2_hd.h | 2 +- deps/nghttp2/lib/nghttp2_hd_huffman.h | 2 +- deps/nghttp2/lib/nghttp2_helper.h | 2 +- deps/nghttp2/lib/nghttp2_http.h | 2 +- deps/nghttp2/lib/nghttp2_int.h | 2 +- deps/nghttp2/lib/nghttp2_map.h | 2 +- deps/nghttp2/lib/nghttp2_mem.h | 2 +- deps/nghttp2/lib/nghttp2_net.h | 16 +- deps/nghttp2/lib/nghttp2_npn.h | 2 +- deps/nghttp2/lib/nghttp2_option.c | 4 + deps/nghttp2/lib/nghttp2_option.h | 2 +- deps/nghttp2/lib/nghttp2_outbound_item.c | 3 + deps/nghttp2/lib/nghttp2_outbound_item.h | 2 +- deps/nghttp2/lib/nghttp2_pq.h | 2 +- deps/nghttp2/lib/nghttp2_priority_spec.h | 2 +- deps/nghttp2/lib/nghttp2_queue.h | 2 +- deps/nghttp2/lib/nghttp2_rcbuf.h | 2 +- deps/nghttp2/lib/nghttp2_session.c | 161 +++++++++++++++++- deps/nghttp2/lib/nghttp2_session.h | 35 +++- deps/nghttp2/lib/nghttp2_stream.h | 2 +- deps/nghttp2/lib/nghttp2_submit.c | 83 +++++++++ deps/nghttp2/lib/nghttp2_submit.h | 2 +- deps/nghttp2/lib/nghttp2_version.c | 2 +- 30 files changed, 599 insertions(+), 78 deletions(-) diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2.h index 14f8950bed8d15..8c54b9c8cc464d 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2.h @@ -28,7 +28,7 @@ /* Define WIN32 when build target is Win32 API (borrowed from libcurl) */ #if (defined(_WIN32) || defined(__WIN32__)) && !defined(WIN32) -#define WIN32 +# define WIN32 #endif #ifdef __cplusplus @@ -40,9 +40,9 @@ extern "C" { /* MSVC < 2013 does not have inttypes.h because it is not C99 compliant. See compiler macros and version number in https://sourceforge.net/p/predef/wiki/Compilers/ */ -#include +# include #else /* !defined(_MSC_VER) || (_MSC_VER >= 1800) */ -#include +# include #endif /* !defined(_MSC_VER) || (_MSC_VER >= 1800) */ #include #include @@ -50,20 +50,20 @@ extern "C" { #include #ifdef NGHTTP2_STATICLIB -#define NGHTTP2_EXTERN +# define NGHTTP2_EXTERN #elif defined(WIN32) -#ifdef BUILDING_NGHTTP2 -#define NGHTTP2_EXTERN __declspec(dllexport) -#else /* !BUILDING_NGHTTP2 */ -#define NGHTTP2_EXTERN __declspec(dllimport) -#endif /* !BUILDING_NGHTTP2 */ -#else /* !defined(WIN32) */ -#ifdef BUILDING_NGHTTP2 -#define NGHTTP2_EXTERN __attribute__((visibility("default"))) -#else /* !BUILDING_NGHTTP2 */ -#define NGHTTP2_EXTERN -#endif /* !BUILDING_NGHTTP2 */ -#endif /* !defined(WIN32) */ +# ifdef BUILDING_NGHTTP2 +# define NGHTTP2_EXTERN __declspec(dllexport) +# else /* !BUILDING_NGHTTP2 */ +# define NGHTTP2_EXTERN __declspec(dllimport) +# endif /* !BUILDING_NGHTTP2 */ +#else /* !defined(WIN32) */ +# ifdef BUILDING_NGHTTP2 +# define NGHTTP2_EXTERN __attribute__((visibility("default"))) +# else /* !BUILDING_NGHTTP2 */ +# define NGHTTP2_EXTERN +# endif /* !BUILDING_NGHTTP2 */ +#endif /* !defined(WIN32) */ /** * @macro @@ -611,7 +611,12 @@ typedef enum { * The ALTSVC frame, which is defined in `RFC 7383 * `_. */ - NGHTTP2_ALTSVC = 0x0a + NGHTTP2_ALTSVC = 0x0a, + /** + * The ORIGIN frame, which is defined by `RFC 8336 + * `_. + */ + NGHTTP2_ORIGIN = 0x0c } nghttp2_frame_type; /** @@ -2473,15 +2478,15 @@ nghttp2_option_set_no_auto_window_update(nghttp2_option *option, int val); * * This option sets the SETTINGS_MAX_CONCURRENT_STREAMS value of * remote endpoint as if it is received in SETTINGS frame. Without - * specifying this option, before the local endpoint receives - * SETTINGS_MAX_CONCURRENT_STREAMS in SETTINGS frame from remote - * endpoint, SETTINGS_MAX_CONCURRENT_STREAMS is unlimited. This may - * cause problem if local endpoint submits lots of requests initially - * and sending them at once to the remote peer may lead to the - * rejection of some requests. Specifying this option to the sensible - * value, say 100, may avoid this kind of issue. This value will be - * overwritten if the local endpoint receives - * SETTINGS_MAX_CONCURRENT_STREAMS from the remote endpoint. + * specifying this option, the maximum number of outgoing concurrent + * streams is initially limited to 100 to avoid issues when the local + * endpoint submits lots of requests before receiving initial SETTINGS + * frame from the remote endpoint, since sending them at once to the + * remote endpoint could lead to rejection of some of the requests. + * This value will be overwritten when the local endpoint receives + * initial SETTINGS frame from the remote endpoint, either to the + * value advertised in SETTINGS_MAX_CONCURRENT_STREAMS or to the + * default value (unlimited) if none was advertised. */ NGHTTP2_EXTERN void nghttp2_option_set_peer_max_concurrent_streams(nghttp2_option *option, @@ -3797,10 +3802,13 @@ nghttp2_priority_spec_check_default(const nghttp2_priority_spec *pri_spec); * .. warning:: * * This function returns assigned stream ID if it succeeds. But - * that stream is not opened yet. The application must not submit + * that stream is not created yet. The application must not submit * frame to that stream ID before * :type:`nghttp2_before_frame_send_callback` is called for this - * frame. + * frame. This means `nghttp2_session_get_stream_user_data()` does + * not work before the callback. But + * `nghttp2_session_set_stream_user_data()` handles this situation + * specially, and it can set data to a stream during this period. * */ NGHTTP2_EXTERN int32_t nghttp2_submit_request( @@ -4516,8 +4524,7 @@ typedef struct { * Submits ALTSVC frame. * * ALTSVC frame is a non-critical extension to HTTP/2, and defined in - * is defined in `RFC 7383 - * `_. + * `RFC 7383 `_. * * The |flags| is currently ignored and should be * :enum:`NGHTTP2_FLAG_NONE`. @@ -4551,6 +4558,81 @@ NGHTTP2_EXTERN int nghttp2_submit_altsvc(nghttp2_session *session, const uint8_t *field_value, size_t field_value_len); +/** + * @struct + * + * The single entry of an origin. + */ +typedef struct { + /** + * The pointer to origin. No validation is made against this field + * by the library. This is not necessarily NULL-terminated. + */ + uint8_t *origin; + /** + * The length of the |origin|. + */ + size_t origin_len; +} nghttp2_origin_entry; + +/** + * @struct + * + * The payload of ORIGIN frame. ORIGIN frame is a non-critical + * extension to HTTP/2 and defined by `RFC 8336 + * `_. + * + * If this frame is received, and + * `nghttp2_option_set_user_recv_extension_type()` is not set, and + * `nghttp2_option_set_builtin_recv_extension_type()` is set for + * :enum:`NGHTTP2_ORIGIN`, ``nghttp2_extension.payload`` will point to + * this struct. + * + * It has the following members: + */ +typedef struct { + /** + * The number of origins contained in |ov|. + */ + size_t nov; + /** + * The pointer to the array of origins contained in ORIGIN frame. + */ + nghttp2_origin_entry *ov; +} nghttp2_ext_origin; + +/** + * @function + * + * Submits ORIGIN frame. + * + * ORIGIN frame is a non-critical extension to HTTP/2 and defined by + * `RFC 8336 `_. + * + * The |flags| is currently ignored and should be + * :enum:`NGHTTP2_FLAG_NONE`. + * + * The |ov| points to the array of origins. The |nov| specifies the + * number of origins included in |ov|. This function creates copies + * of all elements in |ov|. + * + * The ORIGIN frame is only usable by a server. If this function is + * invoked with client side session, this function returns + * :enum:`NGHTTP2_ERR_INVALID_STATE`. + * + * :enum:`NGHTTP2_ERR_NOMEM` + * Out of memory + * :enum:`NGHTTP2_ERR_INVALID_STATE` + * The function is called from client side session. + * :enum:`NGHTTP2_ERR_INVALID_ARGUMENT` + * There are too many origins, or an origin is too large to fit + * into a default frame payload. + */ +NGHTTP2_EXTERN int nghttp2_submit_origin(nghttp2_session *session, + uint8_t flags, + const nghttp2_origin_entry *ov, + size_t nov); + /** * @function * diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h index d32d2754441b25..1f1d4808ca27c0 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h @@ -29,7 +29,7 @@ * @macro * Version number of the nghttp2 library release */ -#define NGHTTP2_VERSION "1.32.0" +#define NGHTTP2_VERSION "1.33.0" /** * @macro @@ -37,6 +37,6 @@ * release. This is a 24 bit number with 8 bits for major number, 8 bits * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203. */ -#define NGHTTP2_VERSION_NUM 0x012000 +#define NGHTTP2_VERSION_NUM 0x012100 #endif /* NGHTTP2VER_H */ diff --git a/deps/nghttp2/lib/nghttp2_buf.h b/deps/nghttp2/lib/nghttp2_buf.h index 9f484a221acb5f..06cce67a11bdea 100644 --- a/deps/nghttp2/lib/nghttp2_buf.h +++ b/deps/nghttp2/lib/nghttp2_buf.h @@ -26,7 +26,7 @@ #define NGHTTP2_BUF_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_callbacks.h b/deps/nghttp2/lib/nghttp2_callbacks.h index b607bbb58b8e3d..61e51fa53638de 100644 --- a/deps/nghttp2/lib/nghttp2_callbacks.h +++ b/deps/nghttp2/lib/nghttp2_callbacks.h @@ -26,7 +26,7 @@ #define NGHTTP2_CALLBACKS_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_debug.h b/deps/nghttp2/lib/nghttp2_debug.h index 2e2cd0d145e5ba..cbb4dd57547234 100644 --- a/deps/nghttp2/lib/nghttp2_debug.h +++ b/deps/nghttp2/lib/nghttp2_debug.h @@ -26,18 +26,18 @@ #define NGHTTP2_DEBUG_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include #ifdef DEBUGBUILD -#define DEBUGF(...) nghttp2_debug_vprintf(__VA_ARGS__) +# define DEBUGF(...) nghttp2_debug_vprintf(__VA_ARGS__) void nghttp2_debug_vprintf(const char *format, ...); #else -#define DEBUGF(...) \ - do { \ - } while (0) +# define DEBUGF(...) \ + do { \ + } while (0) #endif #endif /* NGHTTP2_DEBUG_H */ diff --git a/deps/nghttp2/lib/nghttp2_frame.c b/deps/nghttp2/lib/nghttp2_frame.c index fa7cb6953bc539..6e33f3c247f5cb 100644 --- a/deps/nghttp2/lib/nghttp2_frame.c +++ b/deps/nghttp2/lib/nghttp2_frame.c @@ -223,6 +223,36 @@ void nghttp2_frame_altsvc_free(nghttp2_extension *frame, nghttp2_mem *mem) { nghttp2_mem_free(mem, altsvc->origin); } +void nghttp2_frame_origin_init(nghttp2_extension *frame, + nghttp2_origin_entry *ov, size_t nov) { + nghttp2_ext_origin *origin; + size_t payloadlen = 0; + size_t i; + + for (i = 0; i < nov; ++i) { + payloadlen += 2 + ov[i].origin_len; + } + + nghttp2_frame_hd_init(&frame->hd, payloadlen, NGHTTP2_ORIGIN, + NGHTTP2_FLAG_NONE, 0); + + origin = frame->payload; + origin->ov = ov; + origin->nov = nov; +} + +void nghttp2_frame_origin_free(nghttp2_extension *frame, nghttp2_mem *mem) { + nghttp2_ext_origin *origin; + + origin = frame->payload; + if (origin == NULL) { + return; + } + /* We use the same buffer for all resources pointed by the field of + origin directly or indirectly. */ + nghttp2_mem_free(mem, origin->ov); +} + size_t nghttp2_frame_priority_len(uint8_t flags) { if (flags & NGHTTP2_FLAG_PRIORITY) { return NGHTTP2_PRIORITY_SPECLEN; @@ -746,6 +776,106 @@ int nghttp2_frame_unpack_altsvc_payload2(nghttp2_extension *frame, return 0; } +int nghttp2_frame_pack_origin(nghttp2_bufs *bufs, nghttp2_extension *frame) { + nghttp2_buf *buf; + nghttp2_ext_origin *origin; + nghttp2_origin_entry *orig; + size_t i; + + origin = frame->payload; + + buf = &bufs->head->buf; + + if (nghttp2_buf_avail(buf) < frame->hd.length) { + return NGHTTP2_ERR_FRAME_SIZE_ERROR; + } + + buf->pos -= NGHTTP2_FRAME_HDLEN; + + nghttp2_frame_pack_frame_hd(buf->pos, &frame->hd); + + for (i = 0; i < origin->nov; ++i) { + orig = &origin->ov[i]; + nghttp2_put_uint16be(buf->last, (uint16_t)orig->origin_len); + buf->last += 2; + buf->last = nghttp2_cpymem(buf->last, orig->origin, orig->origin_len); + } + + assert(nghttp2_buf_len(buf) == NGHTTP2_FRAME_HDLEN + frame->hd.length); + + return 0; +} + +int nghttp2_frame_unpack_origin_payload(nghttp2_extension *frame, + const uint8_t *payload, + size_t payloadlen, nghttp2_mem *mem) { + nghttp2_ext_origin *origin; + const uint8_t *p, *end; + uint8_t *dst; + size_t originlen; + nghttp2_origin_entry *ov; + size_t nov = 0; + size_t len = 0; + + origin = frame->payload; + p = payload; + end = p + payloadlen; + + for (; p != end;) { + if (end - p < 2) { + return NGHTTP2_ERR_FRAME_SIZE_ERROR; + } + originlen = nghttp2_get_uint16(p); + p += 2; + if (originlen == 0) { + continue; + } + if (originlen > (size_t)(end - p)) { + return NGHTTP2_ERR_FRAME_SIZE_ERROR; + } + p += originlen; + /* 1 for terminal NULL */ + len += originlen + 1; + ++nov; + } + + if (nov == 0) { + origin->ov = NULL; + origin->nov = 0; + + return 0; + } + + len += nov * sizeof(nghttp2_origin_entry); + + ov = nghttp2_mem_malloc(mem, len); + if (ov == NULL) { + return NGHTTP2_ERR_NOMEM; + } + + origin->ov = ov; + origin->nov = nov; + + dst = (uint8_t *)ov + nov * sizeof(nghttp2_origin_entry); + p = payload; + + for (; p != end;) { + originlen = nghttp2_get_uint16(p); + p += 2; + if (originlen == 0) { + continue; + } + ov->origin = dst; + ov->origin_len = originlen; + dst = nghttp2_cpymem(dst, p, originlen); + *dst++ = '\0'; + p += originlen; + ++ov; + } + + return 0; +} + nghttp2_settings_entry *nghttp2_frame_iv_copy(const nghttp2_settings_entry *iv, size_t niv, nghttp2_mem *mem) { nghttp2_settings_entry *iv_copy; diff --git a/deps/nghttp2/lib/nghttp2_frame.h b/deps/nghttp2/lib/nghttp2_frame.h index 35ca214a4a7a59..615bbf31f5d60d 100644 --- a/deps/nghttp2/lib/nghttp2_frame.h +++ b/deps/nghttp2/lib/nghttp2_frame.h @@ -26,7 +26,7 @@ #define NGHTTP2_FRAME_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include @@ -72,6 +72,7 @@ /* Union of extension frame payload */ typedef union { nghttp2_ext_altsvc altsvc; + nghttp2_ext_origin origin; } nghttp2_ext_frame_payload; void nghttp2_frame_pack_frame_hd(uint8_t *buf, const nghttp2_frame_hd *hd); @@ -392,6 +393,36 @@ int nghttp2_frame_unpack_altsvc_payload2(nghttp2_extension *frame, const uint8_t *payload, size_t payloadlen, nghttp2_mem *mem); +/* + * Packs ORIGIN frame |frame| in wire frame format and store it in + * |bufs|. + * + * The caller must make sure that nghttp2_bufs_reset(bufs) is called + * before calling this function. + * + * This function returns 0 if it succeeds, or one of the following + * negative error codes: + * + * NGHTTP2_ERR_FRAME_SIZE_ERROR + * The length of the frame is too large. + */ +int nghttp2_frame_pack_origin(nghttp2_bufs *bufs, nghttp2_extension *ext); + +/* + * Unpacks ORIGIN wire format into |frame|. The |payload| of length + * |payloadlen| contains the frame payload. + * + * This function returns 0 if it succeeds, or one of the following + * negative error codes: + * + * NGHTTP2_ERR_NOMEM + * Out of memory. + * NGHTTP2_ERR_FRAME_SIZE_ERROR + * The payload is too small. + */ +int nghttp2_frame_unpack_origin_payload(nghttp2_extension *frame, + const uint8_t *payload, + size_t payloadlen, nghttp2_mem *mem); /* * Initializes HEADERS frame |frame| with given values. |frame| takes * ownership of |nva|, so caller must not free it. If |stream_id| is @@ -489,6 +520,24 @@ void nghttp2_frame_altsvc_init(nghttp2_extension *frame, int32_t stream_id, */ void nghttp2_frame_altsvc_free(nghttp2_extension *frame, nghttp2_mem *mem); +/* + * Initializes ORIGIN frame |frame| with given values. This function + * assumes that frame->payload points to nghttp2_ext_origin object. + * Also |ov| and the memory pointed by the field of its elements are + * allocated in single buffer, starting with |ov|. On success, this + * function takes ownership of |ov|, so caller must not free it. + */ +void nghttp2_frame_origin_init(nghttp2_extension *frame, + nghttp2_origin_entry *ov, size_t nov); + +/* + * Frees up resources under |frame|. This function does not free + * nghttp2_ext_origin object pointed by frame->payload. This function + * only frees nghttp2_ext_origin.ov. Therefore, other fields must be + * allocated in the same buffer with ov. + */ +void nghttp2_frame_origin_free(nghttp2_extension *frame, nghttp2_mem *mem); + /* * Returns the number of padding bytes after payload. The total * padding length is given in the |padlen|. The returned value does diff --git a/deps/nghttp2/lib/nghttp2_hd.h b/deps/nghttp2/lib/nghttp2_hd.h index 760bfbc357efdc..c64a1f2b9b406c 100644 --- a/deps/nghttp2/lib/nghttp2_hd.h +++ b/deps/nghttp2/lib/nghttp2_hd.h @@ -26,7 +26,7 @@ #define NGHTTP2_HD_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_hd_huffman.h b/deps/nghttp2/lib/nghttp2_hd_huffman.h index 83323400313509..c6e3942e95f4fc 100644 --- a/deps/nghttp2/lib/nghttp2_hd_huffman.h +++ b/deps/nghttp2/lib/nghttp2_hd_huffman.h @@ -26,7 +26,7 @@ #define NGHTTP2_HD_HUFFMAN_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_helper.h b/deps/nghttp2/lib/nghttp2_helper.h index 4a32564f39dfa3..b1f18ce541ac36 100644 --- a/deps/nghttp2/lib/nghttp2_helper.h +++ b/deps/nghttp2/lib/nghttp2_helper.h @@ -26,7 +26,7 @@ #define NGHTTP2_HELPER_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_http.h b/deps/nghttp2/lib/nghttp2_http.h index ac684c4d9ecbb1..dd057cdb60757f 100644 --- a/deps/nghttp2/lib/nghttp2_http.h +++ b/deps/nghttp2/lib/nghttp2_http.h @@ -26,7 +26,7 @@ #define NGHTTP2_HTTP_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_int.h b/deps/nghttp2/lib/nghttp2_int.h index 30cf7274bc4675..b23585ccb27da2 100644 --- a/deps/nghttp2/lib/nghttp2_int.h +++ b/deps/nghttp2/lib/nghttp2_int.h @@ -26,7 +26,7 @@ #define NGHTTP2_INT_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_map.h b/deps/nghttp2/lib/nghttp2_map.h index 21262488d6d2d4..f6e29e35f2de3f 100644 --- a/deps/nghttp2/lib/nghttp2_map.h +++ b/deps/nghttp2/lib/nghttp2_map.h @@ -26,7 +26,7 @@ #define NGHTTP2_MAP_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_mem.h b/deps/nghttp2/lib/nghttp2_mem.h index 2d1bd6a0f42396..f83dbcb8f9a588 100644 --- a/deps/nghttp2/lib/nghttp2_mem.h +++ b/deps/nghttp2/lib/nghttp2_mem.h @@ -26,7 +26,7 @@ #define NGHTTP2_MEM_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_net.h b/deps/nghttp2/lib/nghttp2_net.h index 587f4189fdba4a..95ffee74a14fc9 100644 --- a/deps/nghttp2/lib/nghttp2_net.h +++ b/deps/nghttp2/lib/nghttp2_net.h @@ -26,15 +26,15 @@ #define NGHTTP2_NET_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #ifdef HAVE_ARPA_INET_H -#include +# include #endif /* HAVE_ARPA_INET_H */ #ifdef HAVE_NETINET_IN_H -#include +# include #endif /* HAVE_NETINET_IN_H */ #include @@ -44,11 +44,11 @@ define inline functions for those function so that we don't have dependeny on that lib. */ -#ifdef _MSC_VER -#define STIN static __inline -#else -#define STIN static inline -#endif +# ifdef _MSC_VER +# define STIN static __inline +# else +# define STIN static inline +# endif STIN uint32_t htonl(uint32_t hostlong) { uint32_t res; diff --git a/deps/nghttp2/lib/nghttp2_npn.h b/deps/nghttp2/lib/nghttp2_npn.h index a481bde3507ce5..c6f1c04b683594 100644 --- a/deps/nghttp2/lib/nghttp2_npn.h +++ b/deps/nghttp2/lib/nghttp2_npn.h @@ -26,7 +26,7 @@ #define NGHTTP2_NPN_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_option.c b/deps/nghttp2/lib/nghttp2_option.c index aec5dcfa8ab542..8946d7dd38cfb8 100644 --- a/deps/nghttp2/lib/nghttp2_option.c +++ b/deps/nghttp2/lib/nghttp2_option.c @@ -86,6 +86,10 @@ void nghttp2_option_set_builtin_recv_extension_type(nghttp2_option *option, option->opt_set_mask |= NGHTTP2_OPT_BUILTIN_RECV_EXT_TYPES; option->builtin_recv_ext_types |= NGHTTP2_TYPEMASK_ALTSVC; return; + case NGHTTP2_ORIGIN: + option->opt_set_mask |= NGHTTP2_OPT_BUILTIN_RECV_EXT_TYPES; + option->builtin_recv_ext_types |= NGHTTP2_TYPEMASK_ORIGIN; + return; default: return; } diff --git a/deps/nghttp2/lib/nghttp2_option.h b/deps/nghttp2/lib/nghttp2_option.h index c743e33b8ed551..29e72aa321007a 100644 --- a/deps/nghttp2/lib/nghttp2_option.h +++ b/deps/nghttp2/lib/nghttp2_option.h @@ -26,7 +26,7 @@ #define NGHTTP2_OPTION_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_outbound_item.c b/deps/nghttp2/lib/nghttp2_outbound_item.c index 1633cc36859da1..f651c8029ac024 100644 --- a/deps/nghttp2/lib/nghttp2_outbound_item.c +++ b/deps/nghttp2/lib/nghttp2_outbound_item.c @@ -86,6 +86,9 @@ void nghttp2_outbound_item_free(nghttp2_outbound_item *item, nghttp2_mem *mem) { case NGHTTP2_ALTSVC: nghttp2_frame_altsvc_free(&frame->ext, mem); break; + case NGHTTP2_ORIGIN: + nghttp2_frame_origin_free(&frame->ext, mem); + break; default: assert(0); break; diff --git a/deps/nghttp2/lib/nghttp2_outbound_item.h b/deps/nghttp2/lib/nghttp2_outbound_item.h index 89a8a92668dd5c..b5f503a312dd8c 100644 --- a/deps/nghttp2/lib/nghttp2_outbound_item.h +++ b/deps/nghttp2/lib/nghttp2_outbound_item.h @@ -26,7 +26,7 @@ #define NGHTTP2_OUTBOUND_ITEM_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_pq.h b/deps/nghttp2/lib/nghttp2_pq.h index 71cf96a14e0c77..2d7b702ac18ad0 100644 --- a/deps/nghttp2/lib/nghttp2_pq.h +++ b/deps/nghttp2/lib/nghttp2_pq.h @@ -26,7 +26,7 @@ #define NGHTTP2_PQ_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_priority_spec.h b/deps/nghttp2/lib/nghttp2_priority_spec.h index 98fac21060091e..92ece822a8f257 100644 --- a/deps/nghttp2/lib/nghttp2_priority_spec.h +++ b/deps/nghttp2/lib/nghttp2_priority_spec.h @@ -26,7 +26,7 @@ #define NGHTTP2_PRIORITY_SPEC_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_queue.h b/deps/nghttp2/lib/nghttp2_queue.h index c7eb753ca92182..a06fa6c7a46fc7 100644 --- a/deps/nghttp2/lib/nghttp2_queue.h +++ b/deps/nghttp2/lib/nghttp2_queue.h @@ -26,7 +26,7 @@ #define NGHTTP2_QUEUE_H #ifdef HAVE_CONFIG_H -#include "config.h" +# include "config.h" #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_rcbuf.h b/deps/nghttp2/lib/nghttp2_rcbuf.h index 29d1543e2c5965..6814e709fb4148 100644 --- a/deps/nghttp2/lib/nghttp2_rcbuf.h +++ b/deps/nghttp2/lib/nghttp2_rcbuf.h @@ -26,7 +26,7 @@ #define NGHTTP2_RCBUF_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c index a9e7a62390e56a..418ad6663585f5 100644 --- a/deps/nghttp2/lib/nghttp2_session.c +++ b/deps/nghttp2/lib/nghttp2_session.c @@ -348,6 +348,12 @@ static void session_inbound_frame_reset(nghttp2_session *session) { } nghttp2_frame_altsvc_free(&iframe->frame.ext, mem); break; + case NGHTTP2_ORIGIN: + if ((session->builtin_recv_ext_types & NGHTTP2_TYPEMASK_ORIGIN) == 0) { + break; + } + nghttp2_frame_origin_free(&iframe->frame.ext, mem); + break; } } @@ -1749,6 +1755,13 @@ static int session_predicate_altsvc_send(nghttp2_session *session, return 0; } +static int session_predicate_origin_send(nghttp2_session *session) { + if (session_is_closing(session)) { + return NGHTTP2_ERR_SESSION_CLOSING; + } + return 0; +} + /* Take into account settings max frame size and both connection-level flow control here */ static ssize_t @@ -2280,6 +2293,18 @@ static int session_prep_frame(nghttp2_session *session, nghttp2_frame_pack_altsvc(&session->aob.framebufs, &frame->ext); + return 0; + case NGHTTP2_ORIGIN: + rv = session_predicate_origin_send(session); + if (rv != 0) { + return rv; + } + + rv = nghttp2_frame_pack_origin(&session->aob.framebufs, &frame->ext); + if (rv != 0) { + return rv; + } + return 0; default: /* Unreachable here */ @@ -4385,6 +4410,12 @@ int nghttp2_session_on_settings_received(nghttp2_session *session, return session_call_on_frame_received(session, frame); } + if (!session->remote_settings_received) { + session->remote_settings.max_concurrent_streams = + NGHTTP2_DEFAULT_MAX_CONCURRENT_STREAMS; + session->remote_settings_received = 1; + } + for (i = 0; i < frame->settings.niv; ++i) { nghttp2_settings_entry *entry = &frame->settings.iv[i]; @@ -4821,6 +4852,11 @@ int nghttp2_session_on_altsvc_received(nghttp2_session *session, return session_call_on_frame_received(session, frame); } +int nghttp2_session_on_origin_received(nghttp2_session *session, + nghttp2_frame *frame) { + return session_call_on_frame_received(session, frame); +} + static int session_process_altsvc_frame(nghttp2_session *session) { nghttp2_inbound_frame *iframe = &session->iframe; nghttp2_frame *frame = &iframe->frame; @@ -4836,6 +4872,25 @@ static int session_process_altsvc_frame(nghttp2_session *session) { return nghttp2_session_on_altsvc_received(session, frame); } +static int session_process_origin_frame(nghttp2_session *session) { + nghttp2_inbound_frame *iframe = &session->iframe; + nghttp2_frame *frame = &iframe->frame; + nghttp2_mem *mem = &session->mem; + int rv; + + rv = nghttp2_frame_unpack_origin_payload(&frame->ext, iframe->lbuf.pos, + nghttp2_buf_len(&iframe->lbuf), mem); + if (rv != 0) { + if (nghttp2_is_fatal(rv)) { + return rv; + } + /* Ignore ORIGIN frame which cannot be parsed. */ + return 0; + } + + return nghttp2_session_on_origin_received(session, frame); +} + static int session_process_extension_frame(nghttp2_session *session) { int rv; nghttp2_inbound_frame *iframe = &session->iframe; @@ -5746,6 +5801,42 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, iframe->state = NGHTTP2_IB_READ_NBYTE; inbound_frame_set_mark(iframe, 2); + break; + case NGHTTP2_ORIGIN: + if (!(session->builtin_recv_ext_types & NGHTTP2_TYPEMASK_ORIGIN)) { + busy = 1; + iframe->state = NGHTTP2_IB_IGN_PAYLOAD; + break; + } + + DEBUGF("recv: ORIGIN\n"); + + iframe->frame.ext.payload = &iframe->ext_frame_payload.origin; + + if (session->server || iframe->frame.hd.stream_id || + (iframe->frame.hd.flags & 0xf0)) { + busy = 1; + iframe->state = NGHTTP2_IB_IGN_PAYLOAD; + break; + } + + iframe->frame.hd.flags = NGHTTP2_FLAG_NONE; + + if (iframe->payloadleft) { + iframe->raw_lbuf = nghttp2_mem_malloc(mem, iframe->payloadleft); + + if (iframe->raw_lbuf == NULL) { + return NGHTTP2_ERR_NOMEM; + } + + nghttp2_buf_wrap_init(&iframe->lbuf, iframe->raw_lbuf, + iframe->payloadleft); + } else { + busy = 1; + } + + iframe->state = NGHTTP2_IB_READ_ORIGIN_PAYLOAD; + break; default: busy = 1; @@ -6583,7 +6674,6 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, DEBUGF("recv: [IB_READ_ALTSVC_PAYLOAD]\n"); readlen = inbound_frame_payload_readlen(iframe, in, last); - if (readlen > 0) { iframe->lbuf.last = nghttp2_cpymem(iframe->lbuf.last, in, readlen); @@ -6601,11 +6691,44 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, } rv = session_process_altsvc_frame(session); + if (nghttp2_is_fatal(rv)) { + return rv; + } + + session_inbound_frame_reset(session); + + break; + case NGHTTP2_IB_READ_ORIGIN_PAYLOAD: + DEBUGF("recv: [IB_READ_ORIGIN_PAYLOAD]\n"); + + readlen = inbound_frame_payload_readlen(iframe, in, last); + + if (readlen > 0) { + iframe->lbuf.last = nghttp2_cpymem(iframe->lbuf.last, in, readlen); + + iframe->payloadleft -= readlen; + in += readlen; + } + + DEBUGF("recv: readlen=%zu, payloadleft=%zu\n", readlen, + iframe->payloadleft); + + if (iframe->payloadleft) { + assert(nghttp2_buf_avail(&iframe->lbuf) > 0); + + break; + } + + rv = session_process_origin_frame(session); if (nghttp2_is_fatal(rv)) { return rv; } + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + session_inbound_frame_reset(session); break; @@ -7085,12 +7208,42 @@ int nghttp2_session_set_stream_user_data(nghttp2_session *session, int32_t stream_id, void *stream_user_data) { nghttp2_stream *stream; + nghttp2_frame *frame; + nghttp2_outbound_item *item; + stream = nghttp2_session_get_stream(session, stream_id); - if (!stream) { + if (stream) { + stream->stream_user_data = stream_user_data; + return 0; + } + + if (session->server || !nghttp2_session_is_my_stream_id(session, stream_id) || + !nghttp2_outbound_queue_top(&session->ob_syn)) { return NGHTTP2_ERR_INVALID_ARGUMENT; } - stream->stream_user_data = stream_user_data; - return 0; + + frame = &nghttp2_outbound_queue_top(&session->ob_syn)->frame; + assert(frame->hd.type == NGHTTP2_HEADERS); + + if (frame->hd.stream_id > stream_id || + (uint32_t)stream_id >= session->next_stream_id) { + return NGHTTP2_ERR_INVALID_ARGUMENT; + } + + for (item = session->ob_syn.head; item; item = item->qnext) { + if (item->frame.hd.stream_id < stream_id) { + continue; + } + + if (item->frame.hd.stream_id > stream_id) { + break; + } + + item->aux_data.headers.stream_user_data = stream_user_data; + return 0; + } + + return NGHTTP2_ERR_INVALID_ARGUMENT; } int nghttp2_session_resume_data(nghttp2_session *session, int32_t stream_id) { diff --git a/deps/nghttp2/lib/nghttp2_session.h b/deps/nghttp2/lib/nghttp2_session.h index c7cb27d77c1e25..5add50bc8bce16 100644 --- a/deps/nghttp2/lib/nghttp2_session.h +++ b/deps/nghttp2/lib/nghttp2_session.h @@ -26,7 +26,7 @@ #define NGHTTP2_SESSION_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include @@ -61,7 +61,8 @@ typedef enum { */ typedef enum { NGHTTP2_TYPEMASK_NONE = 0, - NGHTTP2_TYPEMASK_ALTSVC = 1 << 0 + NGHTTP2_TYPEMASK_ALTSVC = 1 << 0, + NGHTTP2_TYPEMASK_ORIGIN = 1 << 1 } nghttp2_typemask; typedef enum { @@ -121,6 +122,7 @@ typedef enum { NGHTTP2_IB_IGN_DATA, NGHTTP2_IB_IGN_ALL, NGHTTP2_IB_READ_ALTSVC_PAYLOAD, + NGHTTP2_IB_READ_ORIGIN_PAYLOAD, NGHTTP2_IB_READ_EXTENSION_PAYLOAD } nghttp2_inbound_state; @@ -301,8 +303,10 @@ struct nghttp2_session { increased/decreased by submitting WINDOW_UPDATE. See nghttp2_submit_window_update(). */ int32_t local_window_size; - /* Settings value received from the remote endpoint. We just use ID - as index. The index = 0 is unused. */ + /* This flag is used to indicate that the local endpoint received initial + SETTINGS frame from the remote endpoint. */ + uint8_t remote_settings_received; + /* Settings value received from the remote endpoint. */ nghttp2_settings_storage remote_settings; /* Settings value of the local endpoint. */ nghttp2_settings_storage local_settings; @@ -698,7 +702,7 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session, * NGHTTP2_ERR_NOMEM * Out of memory. * NGHTTP2_ERR_CALLBACK_FAILURE - * The callback function failed. + * The callback function failed. * NGHTTP2_ERR_FLOODED * There are too many items in outbound queue, and this is most * likely caused by misbehaviour of peer. @@ -716,7 +720,7 @@ int nghttp2_session_on_ping_received(nghttp2_session *session, * NGHTTP2_ERR_NOMEM * Out of memory. * NGHTTP2_ERR_CALLBACK_FAILURE - * The callback function failed. + * The callback function failed. */ int nghttp2_session_on_goaway_received(nghttp2_session *session, nghttp2_frame *frame); @@ -731,7 +735,7 @@ int nghttp2_session_on_goaway_received(nghttp2_session *session, * NGHTTP2_ERR_NOMEM * Out of memory. * NGHTTP2_ERR_CALLBACK_FAILURE - * The callback function failed. + * The callback function failed. */ int nghttp2_session_on_window_update_received(nghttp2_session *session, nghttp2_frame *frame); @@ -744,11 +748,24 @@ int nghttp2_session_on_window_update_received(nghttp2_session *session, * negative error codes: * * NGHTTP2_ERR_CALLBACK_FAILURE - * The callback function failed. + * The callback function failed. */ int nghttp2_session_on_altsvc_received(nghttp2_session *session, nghttp2_frame *frame); +/* + * Called when ORIGIN is received, assuming |frame| is properly + * initialized. + * + * This function returns 0 if it succeeds, or one of the following + * negative error codes: + * + * NGHTTP2_ERR_CALLBACK_FAILURE + * The callback function failed. + */ +int nghttp2_session_on_origin_received(nghttp2_session *session, + nghttp2_frame *frame); + /* * Called when DATA is received, assuming |frame| is properly * initialized. @@ -759,7 +776,7 @@ int nghttp2_session_on_altsvc_received(nghttp2_session *session, * NGHTTP2_ERR_NOMEM * Out of memory. * NGHTTP2_ERR_CALLBACK_FAILURE - * The callback function failed. + * The callback function failed. */ int nghttp2_session_on_data_received(nghttp2_session *session, nghttp2_frame *frame); diff --git a/deps/nghttp2/lib/nghttp2_stream.h b/deps/nghttp2/lib/nghttp2_stream.h index da0e5d532c2f0b..d1d5856d800e76 100644 --- a/deps/nghttp2/lib/nghttp2_stream.h +++ b/deps/nghttp2/lib/nghttp2_stream.h @@ -26,7 +26,7 @@ #define NGHTTP2_STREAM_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_submit.c b/deps/nghttp2/lib/nghttp2_submit.c index 6c15c82488e724..f604eff5c9017f 100644 --- a/deps/nghttp2/lib/nghttp2_submit.c +++ b/deps/nghttp2/lib/nghttp2_submit.c @@ -571,6 +571,89 @@ int nghttp2_submit_altsvc(nghttp2_session *session, uint8_t flags, return rv; } +int nghttp2_submit_origin(nghttp2_session *session, uint8_t flags, + const nghttp2_origin_entry *ov, size_t nov) { + nghttp2_mem *mem; + uint8_t *p; + nghttp2_outbound_item *item; + nghttp2_frame *frame; + nghttp2_ext_origin *origin; + nghttp2_origin_entry *ov_copy; + size_t len = 0; + size_t i; + int rv; + (void)flags; + + mem = &session->mem; + + if (!session->server) { + return NGHTTP2_ERR_INVALID_STATE; + } + + if (nov) { + for (i = 0; i < nov; ++i) { + len += ov[i].origin_len; + } + + if (2 * nov + len > NGHTTP2_MAX_PAYLOADLEN) { + return NGHTTP2_ERR_INVALID_ARGUMENT; + } + + /* The last nov is added for terminal NULL character. */ + ov_copy = + nghttp2_mem_malloc(mem, nov * sizeof(nghttp2_origin_entry) + len + nov); + if (ov_copy == NULL) { + return NGHTTP2_ERR_NOMEM; + } + + p = (uint8_t *)ov_copy + nov * sizeof(nghttp2_origin_entry); + + for (i = 0; i < nov; ++i) { + ov_copy[i].origin = p; + ov_copy[i].origin_len = ov[i].origin_len; + p = nghttp2_cpymem(p, ov[i].origin, ov[i].origin_len); + *p++ = '\0'; + } + + assert((size_t)(p - (uint8_t *)ov_copy) == + nov * sizeof(nghttp2_origin_entry) + len + nov); + } else { + ov_copy = NULL; + } + + item = nghttp2_mem_malloc(mem, sizeof(nghttp2_outbound_item)); + if (item == NULL) { + rv = NGHTTP2_ERR_NOMEM; + goto fail_item_malloc; + } + + nghttp2_outbound_item_init(item); + + item->aux_data.ext.builtin = 1; + + origin = &item->ext_frame_payload.origin; + + frame = &item->frame; + frame->ext.payload = origin; + + nghttp2_frame_origin_init(&frame->ext, ov_copy, nov); + + rv = nghttp2_session_add_item(session, item); + if (rv != 0) { + nghttp2_frame_origin_free(&frame->ext, mem); + nghttp2_mem_free(mem, item); + + return rv; + } + + return 0; + +fail_item_malloc: + free(ov_copy); + + return rv; +} + static uint8_t set_request_flags(const nghttp2_priority_spec *pri_spec, const nghttp2_data_provider *data_prd) { uint8_t flags = NGHTTP2_FLAG_NONE; diff --git a/deps/nghttp2/lib/nghttp2_submit.h b/deps/nghttp2/lib/nghttp2_submit.h index 545388cfa3bef4..74d702fbcf077e 100644 --- a/deps/nghttp2/lib/nghttp2_submit.h +++ b/deps/nghttp2/lib/nghttp2_submit.h @@ -26,7 +26,7 @@ #define NGHTTP2_SUBMIT_H #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include diff --git a/deps/nghttp2/lib/nghttp2_version.c b/deps/nghttp2/lib/nghttp2_version.c index 8c5710d419c331..4211f2cf8f624d 100644 --- a/deps/nghttp2/lib/nghttp2_version.c +++ b/deps/nghttp2/lib/nghttp2_version.c @@ -23,7 +23,7 @@ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ #ifdef HAVE_CONFIG_H -#include +# include #endif /* HAVE_CONFIG_H */ #include From 62ce017015c6e3173ac6e5c0f7af19dd345f65be Mon Sep 17 00:00:00 2001 From: Szymon Marczak Date: Sun, 2 Sep 2018 12:07:20 +0200 Subject: [PATCH 58/67] http2: don't expose the original socket through the socket proxy Refs: https://github.com/nodejs/node/pull/22486 PR-URL: https://github.com/nodejs/node/pull/22650 Reviewed-By: Matteo Collina Reviewed-By: Luigi Pinca Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell --- lib/internal/http2/core.js | 14 +++++++++-- test/parallel/test-http2-socket-proxy.js | 11 ++++++++ .../test-http2-unbound-socket-proxy.js | 25 ------------------- 3 files changed, 23 insertions(+), 27 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 9aa7a0e2a2f79e..575e4152c94228 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -630,7 +630,9 @@ const proxySocketHandler = { get(session, prop) { switch (prop) { case 'setTimeout': - return session.setTimeout.bind(session); + case 'ref': + case 'unref': + return session[prop].bind(session); case 'destroy': case 'emit': case 'end': @@ -638,6 +640,9 @@ const proxySocketHandler = { case 'read': case 'resume': case 'write': + case 'setEncoding': + case 'setKeepAlive': + case 'setNoDelay': throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION'); default: const socket = session[kSocket]; @@ -656,7 +661,9 @@ const proxySocketHandler = { set(session, prop, value) { switch (prop) { case 'setTimeout': - session.setTimeout = value; + case 'ref': + case 'unref': + session[prop] = value; return true; case 'destroy': case 'emit': @@ -665,6 +672,9 @@ const proxySocketHandler = { case 'read': case 'resume': case 'write': + case 'setEncoding': + case 'setKeepAlive': + case 'setNoDelay': throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION'); default: const socket = session[kSocket]; diff --git a/test/parallel/test-http2-socket-proxy.js b/test/parallel/test-http2-socket-proxy.js index 17830495addc63..209c49e719c676 100644 --- a/test/parallel/test-http2-socket-proxy.js +++ b/test/parallel/test-http2-socket-proxy.js @@ -38,6 +38,9 @@ server.on('stream', common.mustCall(function(stream, headers) { common.expectsError(() => socket.read, errMsg); common.expectsError(() => socket.resume, errMsg); common.expectsError(() => socket.write, errMsg); + common.expectsError(() => socket.setEncoding, errMsg); + common.expectsError(() => socket.setKeepAlive, errMsg); + common.expectsError(() => socket.setNoDelay, errMsg); common.expectsError(() => (socket.destroy = undefined), errMsg); common.expectsError(() => (socket.emit = undefined), errMsg); @@ -46,10 +49,18 @@ server.on('stream', common.mustCall(function(stream, headers) { common.expectsError(() => (socket.read = undefined), errMsg); common.expectsError(() => (socket.resume = undefined), errMsg); common.expectsError(() => (socket.write = undefined), errMsg); + common.expectsError(() => (socket.setEncoding = undefined), errMsg); + common.expectsError(() => (socket.setKeepAlive = undefined), errMsg); + common.expectsError(() => (socket.setNoDelay = undefined), errMsg); assert.doesNotThrow(() => (socket.on = socket.on)); assert.doesNotThrow(() => (socket.once = socket.once)); + socket.unref(); + assert.strictEqual(socket._handle.hasRef(), false); + socket.ref(); + assert.strictEqual(socket._handle.hasRef(), true); + stream.respond(); socket.writable = 0; diff --git a/test/parallel/test-http2-unbound-socket-proxy.js b/test/parallel/test-http2-unbound-socket-proxy.js index 44f113bac962f7..18881574f2c09b 100644 --- a/test/parallel/test-http2-unbound-socket-proxy.js +++ b/test/parallel/test-http2-unbound-socket-proxy.js @@ -39,31 +39,6 @@ server.listen(0, common.mustCall(() => { }, { code: 'ERR_HTTP2_SOCKET_UNBOUND' }); - common.expectsError(() => { - socket.ref(); - }, { - code: 'ERR_HTTP2_SOCKET_UNBOUND' - }); - common.expectsError(() => { - socket.unref(); - }, { - code: 'ERR_HTTP2_SOCKET_UNBOUND' - }); - common.expectsError(() => { - socket.setEncoding(); - }, { - code: 'ERR_HTTP2_SOCKET_UNBOUND' - }); - common.expectsError(() => { - socket.setKeepAlive(); - }, { - code: 'ERR_HTTP2_SOCKET_UNBOUND' - }); - common.expectsError(() => { - socket.setNoDelay(); - }, { - code: 'ERR_HTTP2_SOCKET_UNBOUND' - }); })); })); })); From f9cd2d5419d2e418fcc34b4111086ef8c9b3bbb9 Mon Sep 17 00:00:00 2001 From: Sagi Tsofan Date: Tue, 11 Sep 2018 01:14:10 +0300 Subject: [PATCH 59/67] doc: document http2 timeouts New default timeout values of "2 minutes" were added into documentation inside 2 classes under "Event: 'timeout'": 1) Class: Http2SecureServer 2) Class: Http2Server New sections for `.setTimeout()` method were added inside `Http2SecureServer` & `Http2Server` docs. PR-URL: https://github.com/nodejs/node/pull/22798 Reviewed-By: Matteo Collina Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- doc/api/http2.md | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/doc/api/http2.md b/doc/api/http2.md index 7a8abcf4b58498..81d1c8fa8e08e7 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -1604,6 +1604,7 @@ added: v8.4.0 The `'timeout'` event is emitted when there is no activity on the Server for a given number of milliseconds set using `http2server.setTimeout()`. +**Default:** 2 minutes. #### server.close([callback]) + +* `msecs` {number} **Default:** `120000` (2 minutes) +* `callback` {Function} +* Returns: {Http2Server} + +Used to set the timeout value for http2 server requests, +and sets a callback function that is called when there is no activity +on the `Http2Server` after `msecs` milliseconds. + +The given callback is registered as a listener on the `'timeout'` event. + +In case of no callback function were assigned, a new `ERR_INVALID_CALLBACK` +error will be thrown. + ### Class: Http2SecureServer + +* `msecs` {number} **Default:** `120000` (2 minutes) +* `callback` {Function} +* Returns: {Http2SecureServer} + +Used to set the timeout value for http2 secure server requests, +and sets a callback function that is called when there is no activity +on the `Http2SecureServer` after `msecs` milliseconds. + +The given callback is registered as a listener on the `'timeout'` event. + +In case of no callback function were assigned, a new `ERR_INVALID_CALLBACK` +error will be thrown. + ### http2.createServer(options[, onRequestHandler]) + +* {boolean} + +Set the `true` if the `END_STREAM` flag was set in the request or response +HEADERS frame received, indicating that no additional data should be received +and the readable side of the `Http2Stream` will be closed. + #### http2stream.pending + +* `origins` { string | URL | Object } One or more URL Strings passed as + separate arguments. + +Submits an `ORIGIN` frame (as defined by [RFC 8336][]) to the connected client +to advertise the set of origins for which the server is capable of providing +authoritative responses. + +```js +const http2 = require('http2'); +const options = getSecureOptionsSomehow(); +const server = http2.createSecureServer(options); +server.on('stream', (stream) => { + stream.respond(); + stream.end('ok'); +}); +server.on('session', (session) => { + session.origin('https://example.com', 'https://example.org'); +}); +``` + +When a string is passed as an `origin`, it will be parsed as a URL and the +origin will be derived. For instance, the origin for the HTTP URL +`'https://example.org/foo/bar'` is the ASCII string +`'https://example.org'`. An error will be thrown if either the given string +cannot be parsed as a URL or if a valid origin cannot be derived. + +A `URL` object, or any object with an `origin` property, may be passed as +an `origin`, in which case the value of the `origin` property will be +used. The value of the `origin` property *must* be a properly serialized +ASCII origin. + +Alternatively, the `origins` option may be used when creating a new HTTP/2 +server using the `http2.createSecureServer()` method: + +```js +const http2 = require('http2'); +const options = getSecureOptionsSomehow(); +options.origins = ['https://example.com', 'https://example.org']; +const server = http2.createSecureServer(options); +server.on('stream', (stream) => { + stream.respond(); + stream.end('ok'); +}); +``` + ### Class: ClientHttp2Session + +* `origins` {string[]} + +The `'origin'` event is emitted whenever an `ORIGIN` frame is received by +the client. The event is emitted with an array of `origin` strings. The +`http2session.originSet` will be updated to include the received +origins. + +```js +const http2 = require('http2'); +const client = http2.connect('https://example.org'); + +client.on('origin', (origins) => { + for (let n = 0; n < origins.length; n++) + console.log(origins[n]); +}); +``` + +The `'origin'` event is only emitted when using a secure TLS connection. + #### clienthttp2session.request(headers[, options]) + +* `payload` {Buffer} The `PING` frame 8-byte payload + +The `'ping'` event is emitted whenever a `PING` frame is received from the +connected peer. + #### Event: 'remoteSettings' -> Stability: 1 - Experimental +> Stability: 2 - Stable The `http2` module provides an implementation of the [HTTP/2][] protocol. It can be accessed using: diff --git a/lib/http2.js b/lib/http2.js index de06de1cc414cb..1f770ff4c734cd 100644 --- a/lib/http2.js +++ b/lib/http2.js @@ -1,11 +1,5 @@ 'use strict'; -process.emitWarning( - 'The http2 module is an experimental API.', - 'ExperimentalWarning', undefined, - 'See https://github.com/nodejs/http2' -); - const { constants, getDefaultSettings,