From 41b9978841b336535d9526589ee4583a6ab68b1a Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Mon, 11 Dec 2017 15:05:18 -0800 Subject: [PATCH 1/6] http2: implement ref() and unref() on client sessions --- doc/api/http2.md | 153 ++++++++++-------- lib/internal/http2/core.js | 12 ++ .../parallel/test-http2-client-session-ref.js | 29 ++++ 3 files changed, 129 insertions(+), 65 deletions(-) create mode 100644 test/parallel/test-http2-client-session-ref.js diff --git a/doc/api/http2.md b/doc/api/http2.md index 8facdf465b2db7..cba7d1a51a7736 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -423,69 +423,6 @@ added: v8.4.0 A prototype-less object describing the current remote settings of this `Http2Session`. The remote settings are set by the *connected* HTTP/2 peer. -#### http2session.request(headers[, options]) - - -* `headers` {[Headers Object][]} -* `options` {Object} - * `endStream` {boolean} `true` if the `Http2Stream` *writable* side should - be closed initially, such as when sending a `GET` request that should not - expect a payload body. - * `exclusive` {boolean} When `true` and `parent` identifies a parent Stream, - the created stream is made the sole direct dependency of the parent, with - all other existing dependents made a dependent of the newly created stream. - **Default:** `false` - * `parent` {number} Specifies the numeric identifier of a stream the newly - created stream is dependent on. - * `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. - -* Returns: {ClientHttp2Stream} - -For HTTP/2 Client `Http2Session` instances only, the `http2session.request()` -creates and returns an `Http2Stream` instance that can be used to send an -HTTP/2 request to the connected server. - -This method is only available if `http2session.type` is equal to -`http2.constants.NGHTTP2_SESSION_CLIENT`. - -```js -const http2 = require('http2'); -const clientSession = http2.connect('https://localhost:1234'); -const { - HTTP2_HEADER_PATH, - HTTP2_HEADER_STATUS -} = http2.constants; - -const req = clientSession.request({ [HTTP2_HEADER_PATH]: '/' }); -req.on('response', (headers) => { - console.log(headers[HTTP2_HEADER_STATUS]); - req.on('data', (chunk) => { /** .. **/ }); - req.on('end', () => { /** .. **/ }); -}); -``` - -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 used to specify -the trailing header fields to send 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. - -The the `:method` and `:path` pseudoheaders are not specified within `headers`, -they respectively default to: - -* `:method` = `'GET'` -* `:path` = `/` - #### http2session.setTimeout(msecs, callback) + +#### clienthttp2session.ref() + + +Calls [`ref()`][`net.Socket.prototype.ref`] on this `ClientHttp2Session` +instance's underlying [`net.Socket`]. + +#### clienthttp2session.request(headers[, options]) + + +* `headers` {[Headers Object][]} +* `options` {Object} + * `endStream` {boolean} `true` if the `Http2Stream` *writable* side should + be closed initially, such as when sending a `GET` request that should not + expect a payload body. + * `exclusive` {boolean} When `true` and `parent` identifies a parent Stream, + the created stream is made the sole direct dependency of the parent, with + all other existing dependents made a dependent of the newly created stream. + **Default:** `false` + * `parent` {number} Specifies the numeric identifier of a stream the newly + created stream is dependent on. + * `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. + +* Returns: {ClientHttp2Stream} + +For HTTP/2 Client `Http2Session` instances only, the `http2session.request()` +creates and returns an `Http2Stream` instance that can be used to send an +HTTP/2 request to the connected server. + +This method is only available if `http2session.type` is equal to +`http2.constants.NGHTTP2_SESSION_CLIENT`. + +```js +const http2 = require('http2'); +const clientSession = http2.connect('https://localhost:1234'); +const { + HTTP2_HEADER_PATH, + HTTP2_HEADER_STATUS +} = http2.constants; + +const req = clientSession.request({ [HTTP2_HEADER_PATH]: '/' }); +req.on('response', (headers) => { + console.log(headers[HTTP2_HEADER_STATUS]); + req.on('data', (chunk) => { /** .. **/ }); + req.on('end', () => { /** .. **/ }); +}); +``` + +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 used to specify +the trailing header fields to send 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. + +The the `:method` and `:path` pseudoheaders are not specified within `headers`, +they respectively default to: + +* `:method` = `'GET'` +* `:path` = `/` + +#### clienthttp2session.unref() + + +Calls [`unref()`][`net.Socket.prototype.unref`] on this `ClientHttp2Session` +instance's underlying [`net.Socket`]. + ### Class: Http2Stream Calls [`ref()`][`net.Socket.prototype.ref`] on this `ClientHttp2Session` @@ -620,7 +620,7 @@ they respectively default to: #### clienthttp2session.unref() Calls [`unref()`][`net.Socket.prototype.unref`] on this `ClientHttp2Session` diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index f261bb8df2a2d8..192f795679d60f 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1225,13 +1225,19 @@ class ClientHttp2Session extends Http2Session { ref() { if (this[kSocket]) { - this[kSocket].ref(); + assert(this[kSocket]._handle); + if (typeof this[kSocket]._handle.ref === 'function') { + this[kSocket].ref(); + } } } unref() { if (this[kSocket]) { - this[kSocket].unref(); + assert(this[kSocket]._handle); + if (typeof this[kSocket]._handle.unref === 'function') { + this[kSocket].unref(); + } } } } diff --git a/test/parallel/test-http2-client-session-ref.js b/test/parallel/test-http2-client-session-ref.js index 9d37b2651cad58..cccff1f2b0d8ed 100644 --- a/test/parallel/test-http2-client-session-ref.js +++ b/test/parallel/test-http2-client-session-ref.js @@ -7,6 +7,7 @@ if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); const http2 = require('http2'); +const { PassThrough } = require('stream'); function isActiveHandle(h) { return process._getActiveHandles().indexOf(h) !== -1; @@ -15,7 +16,8 @@ function isActiveHandle(h) { const server = http2.createServer(); server.listen(0, common.mustCall(() => { const port = server.address().port; - const client = http2.connect(`http://localhost:${port}`); + + let client = http2.connect(`http://localhost:${port}`); assert.ok(isActiveHandle(client[kSocket])); client.unref(); assert.ok(!isActiveHandle(client[kSocket])); @@ -23,7 +25,20 @@ server.listen(0, common.mustCall(() => { assert.ok(isActiveHandle(client[kSocket])); client.destroy(); assert.ok(!isActiveHandle(client[kSocket])); + // Ensure that calling these methods don't throw after session is destroyed. assert.doesNotThrow(client.unref.bind(client)); assert.doesNotThrow(client.ref.bind(client)); + assert.ok(!isActiveHandle(client[kSocket])); + server.close(); + + // Try with generic DuplexStream + client = http2.connect(`http://localhost:${port}`, { + createConnection: common.mustCall(() => PassThrough()) + }); + assert.doesNotThrow(client.unref.bind(client)); + assert.doesNotThrow(client.ref.bind(client)); + client.destroy(); + assert.doesNotThrow(client.unref.bind(client)); + assert.doesNotThrow(client.ref.bind(client)); })); From d8c901768c2382a6ccd646a9cd292beebea81c71 Mon Sep 17 00:00:00 2001 From: Kelvin Jin Date: Tue, 12 Dec 2017 10:54:23 -0800 Subject: [PATCH 3/6] make unref a property of http2session --- doc/api/http2.md | 32 +++++++------- lib/internal/http2/core.js | 36 ++++++++-------- ...ion-ref.js => test-http2-session-unref.js} | 42 +++++++++++-------- 3 files changed, 59 insertions(+), 51 deletions(-) rename test/parallel/{test-http2-client-session-ref.js => test-http2-session-unref.js} (51%) diff --git a/doc/api/http2.md b/doc/api/http2.md index 619d3918d94fa9..f49c1152b23fa0 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -413,6 +413,14 @@ session.ping(Buffer.from('abcdefgh'), (err, duration, payload) => { If the `payload` argument is not specified, the default payload will be the 64-bit timestamp (little endian) marking the start of the `PING` duration. +#### http2session.ref() + + +Calls [`ref()`][`net.Socket.prototype.ref`] on this `Http2Session` +instance's underlying [`net.Socket`]. + #### http2session.remoteSettings - -#### clienthttp2session.ref() +#### http2session.unref() -Calls [`ref()`][`net.Socket.prototype.ref`] on this `ClientHttp2Session` +Calls [`unref()`][`net.Socket.prototype.unref`] on this `Http2Session` instance's underlying [`net.Socket`]. +### Class: ClientHttp2Session + + #### clienthttp2session.request(headers[, options]) - -Calls [`unref()`][`net.Socket.prototype.unref`] on this `ClientHttp2Session` -instance's underlying [`net.Socket`]. - ### Class: Http2Stream