diff --git a/doc/api/errors.md b/doc/api/errors.md index b78c6b36d6..6de2e6f3af 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -810,8 +810,8 @@ reached. ### ERR_HTTP2_NO_SOCKET_MANIPULATION -Used when attempting to read, write, pause, and/or resume a socket attached to -an `Http2Session`. +Used when attempting to directly manipulate (e.g read, write, pause, resume, +etc.) a socket attached to an `Http2Session`. ### ERR_HTTP2_OUT_OF_STREAMS diff --git a/doc/api/http2.md b/doc/api/http2.md index e6261ad62e..abf0ac5b52 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -463,12 +463,16 @@ added: v8.4.0 * Value: {net.Socket|tls.TLSSocket} -A reference to the [`net.Socket`][] or [`tls.TLSSocket`][] to which this -`Http2Session` instance is bound. +Returns a Proxy object that acts as a `net.Socket` (or `tls.TLSSocket`) but +limits available methods to ones safe to use with HTTP/2. -*Note*: It is not recommended for user code to interact directly with a -`Socket` bound to an `Http2Session`. See [Http2Session and Sockets][] for -details. +`destroy`, `emit`, `end`, `pause`, `read`, `resume`, and `write` will throw +an error with code `ERR_HTTP2_NO_SOCKET_MANIPULATION`. See +[Http2Session and Sockets][] for more information. + +`setTimeout` method will be called on this `Http2Session`. + +All other interactions will be routed directly to the socket. #### http2session.state -* {net.Socket} +* {net.Socket|tls.TLSSocket} -Returns a Proxy object that acts as a `net.Socket` but applies getters, -setters and methods based on HTTP/2 logic. +Returns a Proxy object that acts as a `net.Socket` (or `tls.TLSSocket`) but +applies getters, setters and methods based on HTTP/2 logic. `destroyed`, `readable`, and `writable` properties will be retrieved from and set on `request.stream`. @@ -2293,7 +2297,7 @@ will result in a [`TypeError`][] being thrown. added: v8.4.0 --> -* {net.Socket} +* {net.Socket|tls.TLSSocket} See [`response.socket`][]. @@ -2510,10 +2514,10 @@ Returns `response`. added: v8.4.0 --> -* {net.Socket} +* {net.Socket|tls.TLSSocket} -Returns a Proxy object that acts as a `net.Socket` but applies getters, -setters and methods based on HTTP/2 logic. +Returns a Proxy object that acts as a `net.Socket` (or `tls.TLSSocket`) but +applies getters, setters and methods based on HTTP/2 logic. `destroyed`, `readable`, and `writable` properties will be retrieved from and set on `response.stream`. diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 408a0e9507..b8bda9d2b2 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -208,8 +208,7 @@ E('ERR_HTTP2_INVALID_STREAM', 'The stream has been destroyed'); E('ERR_HTTP2_MAX_PENDING_SETTINGS_ACK', (max) => `Maximum number of pending settings acknowledgements (${max})`); E('ERR_HTTP2_NO_SOCKET_MANIPULATION', - 'HTTP/2 sockets should not be directly read from, written to, ' + - 'paused and/or resumed.'); + 'HTTP/2 sockets should not be directly manipulated (e.g. read and written)'); E('ERR_HTTP2_OUT_OF_STREAMS', 'No stream ID is available because maximum stream ID has been reached'); E('ERR_HTTP2_PAYLOAD_FORBIDDEN', diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index 84f15b2ed8..c96fc93196 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -5,6 +5,7 @@ const Readable = Stream.Readable; const binding = process.binding('http2'); const constants = binding.constants; const errors = require('internal/errors'); +const { kSocket } = require('internal/http2/util'); const kFinish = Symbol('finish'); const kBeginSend = Symbol('begin-send'); @@ -176,15 +177,15 @@ const proxySocketHandler = { throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION'); default: const ref = stream.session !== undefined ? - stream.session.socket : stream; + stream.session[kSocket] : stream; const value = ref[prop]; return typeof value === 'function' ? value.bind(ref) : value; } }, getPrototypeOf(stream) { if (stream.session !== undefined) - return stream.session.socket.constructor.prototype; - return stream.prototype; + return Reflect.getPrototypeOf(stream.session[kSocket]); + return Reflect.getPrototypeOf(stream); }, set(stream, prop, value) { switch (prop) { @@ -201,9 +202,9 @@ const proxySocketHandler = { case 'setTimeout': const session = stream.session; if (session !== undefined) - session[prop] = value; + session.setTimeout = value; else - stream[prop] = value; + stream.setTimeout = value; return true; case 'write': case 'read': @@ -212,7 +213,7 @@ const proxySocketHandler = { throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION'); default: const ref = stream.session !== undefined ? - stream.session.socket : stream; + stream.session[kSocket] : stream; ref[prop] = value; return true; } diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 8d8f200262..727ca51798 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -38,6 +38,7 @@ const { getSettings, getStreamState, isPayloadMeaningless, + kSocket, mapToHeaders, NghttpError, sessionName, @@ -70,10 +71,10 @@ const kOptions = Symbol('options'); const kOwner = Symbol('owner'); const kProceed = Symbol('proceed'); const kProtocol = Symbol('protocol'); +const kProxySocket = Symbol('proxy-socket'); const kRemoteSettings = Symbol('remote-settings'); const kServer = Symbol('server'); const kSession = Symbol('session'); -const kSocket = Symbol('socket'); const kState = Symbol('state'); const kType = Symbol('type'); @@ -672,6 +673,48 @@ function finishSessionDestroy(self, socket) { debug(`[${sessionName(self[kType])}] nghttp2session destroyed`); } +const proxySocketHandler = { + get(session, prop) { + switch (prop) { + case 'setTimeout': + return session.setTimeout.bind(session); + case 'destroy': + case 'emit': + case 'end': + case 'pause': + case 'read': + case 'resume': + case 'write': + throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION'); + default: + const socket = session[kSocket]; + const value = socket[prop]; + return typeof value === 'function' ? value.bind(socket) : value; + } + }, + getPrototypeOf(session) { + return Reflect.getPrototypeOf(session[kSocket]); + }, + set(session, prop, value) { + switch (prop) { + case 'setTimeout': + session.setTimeout = value; + return true; + case 'destroy': + case 'emit': + case 'end': + case 'pause': + case 'read': + case 'resume': + case 'write': + throw new errors.Error('ERR_HTTP2_NO_SOCKET_MANIPULATION'); + default: + session[kSocket][prop] = value; + return true; + } + } +}; + // 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. class Http2Session extends EventEmitter { @@ -707,6 +750,7 @@ class Http2Session extends EventEmitter { }; this[kType] = type; + this[kProxySocket] = null; this[kSocket] = socket; // Do not use nagle's algorithm @@ -756,7 +800,10 @@ class Http2Session extends EventEmitter { // The socket owned by this session get socket() { - return this[kSocket]; + const proxySocket = this[kProxySocket]; + if (proxySocket === null) + return this[kProxySocket] = new Proxy(this, proxySocketHandler); + return proxySocket; } // The session type @@ -957,6 +1004,7 @@ class Http2Session extends EventEmitter { // Disassociate from the socket and server const socket = this[kSocket]; // socket.pause(); + delete this[kProxySocket]; delete this[kSocket]; delete this[kServer]; @@ -2155,30 +2203,6 @@ function socketDestroy(error) { this.destroy(error); } -function socketOnResume() { - if (this._paused) - return this.pause(); - if (this._handle && !this._handle.reading) { - this._handle.reading = true; - this._handle.readStart(); - } -} - -function socketOnPause() { - if (this._handle && this._handle.reading) { - this._handle.reading = false; - this._handle.readStop(); - } -} - -function socketOnDrain() { - const needPause = 0 > this._writableState.highWaterMark; - if (this._paused && !needPause) { - this._paused = false; - this.resume(); - } -} - // When an Http2Session emits an error, first try to forward it to the // server as a sessionError; failing that, forward it to the socket as // a sessionError; failing that, destroy, remove the error listener, and @@ -2267,9 +2291,6 @@ function connectionListener(socket) { } socket.on('error', socketOnError); - socket.on('resume', socketOnResume); - socket.on('pause', socketOnPause); - socket.on('drain', socketOnDrain); socket.on('close', socketOnClose); // Set up the Session @@ -2426,9 +2447,6 @@ function connect(authority, options, listener) { } socket.on('error', socketOnError); - socket.on('resume', socketOnResume); - socket.on('pause', socketOnPause); - socket.on('drain', socketOnDrain); socket.on('close', socketOnClose); const session = new ClientHttp2Session(options, socket); diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index 4610be7e4d..2426a409cf 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -3,6 +3,8 @@ const binding = process.binding('http2'); const errors = require('internal/errors'); +const kSocket = Symbol('socket'); + const { NGHTTP2_SESSION_CLIENT, NGHTTP2_SESSION_SERVER, @@ -551,6 +553,7 @@ module.exports = { getSettings, getStreamState, isPayloadMeaningless, + kSocket, mapToHeaders, NghttpError, sessionName, diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index f10bf60ce3..8b91f2d210 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/test/parallel/test-http2-client-destroy.js @@ -1,3 +1,5 @@ +// Flags: --expose-internals + 'use strict'; const common = require('../common'); @@ -5,6 +7,7 @@ if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); const h2 = require('http2'); +const { kSocket } = require('internal/http2/util'); { const server = h2.createServer(); @@ -13,7 +16,7 @@ const h2 = require('http2'); common.mustCall(() => { const destroyCallbacks = [ (client) => client.destroy(), - (client) => client.socket.destroy() + (client) => client[kSocket].destroy() ]; let remaining = destroyCallbacks.length; @@ -23,9 +26,9 @@ const h2 = require('http2'); client.on( 'connect', common.mustCall(() => { - const socket = client.socket; + const socket = client[kSocket]; - assert(client.socket, 'client session has associated socket'); + assert(socket, 'client session has associated socket'); assert( !client.destroyed, 'client has not been destroyed before destroy is called' @@ -41,7 +44,7 @@ const h2 = require('http2'); destroyCallback(client); assert( - !client.socket, + !client[kSocket], 'client.socket undefined after destroy is called' ); diff --git a/test/parallel/test-http2-client-socket-destroy.js b/test/parallel/test-http2-client-socket-destroy.js index f56a45cc3b..faf4643b03 100644 --- a/test/parallel/test-http2-client-socket-destroy.js +++ b/test/parallel/test-http2-client-socket-destroy.js @@ -1,9 +1,13 @@ +// Flags: --expose-internals + 'use strict'; const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); const h2 = require('http2'); +const { kSocket } = require('internal/http2/util'); + const body = '

this is some data

'; @@ -32,7 +36,7 @@ server.on('listening', common.mustCall(function() { req.on('response', common.mustCall(() => { // send a premature socket close - client.socket.destroy(); + client[kSocket].destroy(); })); req.on('data', common.mustNotCall()); diff --git a/test/parallel/test-http2-compat-socket-set.js b/test/parallel/test-http2-compat-socket-set.js index 7411fef602..87b7a2b444 100644 --- a/test/parallel/test-http2-compat-socket-set.js +++ b/test/parallel/test-http2-compat-socket-set.js @@ -12,8 +12,8 @@ const h2 = require('http2'); const errMsg = { code: 'ERR_HTTP2_NO_SOCKET_MANIPULATION', type: Error, - message: 'HTTP/2 sockets should not be directly read from, written to, ' + - 'paused and/or resumed.' + message: 'HTTP/2 sockets should not be directly manipulated ' + + '(e.g. read and written)' }; const server = h2.createServer(); diff --git a/test/parallel/test-http2-compat-socket.js b/test/parallel/test-http2-compat-socket.js index 8292232f74..aed74149f7 100644 --- a/test/parallel/test-http2-compat-socket.js +++ b/test/parallel/test-http2-compat-socket.js @@ -14,8 +14,8 @@ const net = require('net'); const errMsg = { code: 'ERR_HTTP2_NO_SOCKET_MANIPULATION', type: Error, - message: 'HTTP/2 sockets should not be directly read from, written to, ' + - 'paused and/or resumed.' + message: 'HTTP/2 sockets should not be directly manipulated ' + + '(e.g. read and written)' }; const server = h2.createServer(); diff --git a/test/parallel/test-http2-create-client-secure-session.js b/test/parallel/test-http2-create-client-secure-session.js index 7447d4e5c6..62a79148dc 100644 --- a/test/parallel/test-http2-create-client-secure-session.js +++ b/test/parallel/test-http2-create-client-secure-session.js @@ -1,3 +1,5 @@ +// Flags: --expose-internals + 'use strict'; const common = require('../common'); @@ -8,6 +10,7 @@ if (!common.hasCrypto) const assert = require('assert'); const fixtures = require('../common/fixtures'); const h2 = require('http2'); +const { kSocket } = require('internal/http2/util'); const tls = require('tls'); function loadKey(keyname) { @@ -15,7 +18,7 @@ function loadKey(keyname) { } function onStream(stream, headers) { - const socket = stream.session.socket; + const socket = stream.session[kSocket]; assert(headers[':authority'].startsWith(socket.servername)); stream.respond({ 'content-type': 'text/html', @@ -55,7 +58,7 @@ function verifySecureSession(key, cert, ca, opts) { assert.strictEqual(jsonData.servername, opts.servername || 'localhost'); assert.strictEqual(jsonData.alpnProtocol, 'h2'); server.close(); - client.socket.destroy(); + client[kSocket].destroy(); })); req.end(); }); diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js index 0218f7357a..8291c41528 100644 --- a/test/parallel/test-http2-server-socket-destroy.js +++ b/test/parallel/test-http2-server-socket-destroy.js @@ -1,10 +1,13 @@ +// Flags: --expose-internals + 'use strict'; const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); -const h2 = require('http2'); const assert = require('assert'); +const h2 = require('http2'); +const { kSocket } = require('internal/http2/util'); const { HTTP2_HEADER_METHOD, @@ -24,7 +27,7 @@ function onStream(stream) { }); stream.write('test'); - const socket = stream.session.socket; + const socket = stream.session[kSocket]; // When the socket is destroyed, the close events must be triggered // on the socket, server and session. diff --git a/test/parallel/test-http2-server-socketerror.js b/test/parallel/test-http2-server-socketerror.js index cad92fd299..24945e531a 100644 --- a/test/parallel/test-http2-server-socketerror.js +++ b/test/parallel/test-http2-server-socketerror.js @@ -1,3 +1,5 @@ +// Flags: --expose-internals + 'use strict'; const common = require('../common'); @@ -5,6 +7,7 @@ if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); const http2 = require('http2'); +const { kSocket } = require('internal/http2/util'); const server = http2.createServer(); server.on('stream', common.mustCall((stream) => { @@ -19,12 +22,12 @@ server.on('session', common.mustCall((session) => { type: Error, message: 'test' })(error); - assert.strictEqual(socket, session.socket); + assert.strictEqual(socket, session[kSocket]); }); const isNotCalled = common.mustNotCall(); session.on('socketError', handler); server.on('socketError', isNotCalled); - session.socket.emit('error', new Error('test')); + session[kSocket].emit('error', new Error('test')); session.removeListener('socketError', handler); server.removeListener('socketError', isNotCalled); @@ -35,10 +38,10 @@ server.on('session', common.mustCall((session) => { type: Error, message: 'test' })(error); - assert.strictEqual(socket, session.socket); + assert.strictEqual(socket, session[kSocket]); assert.strictEqual(session, session); })); - session.socket.emit('error', new Error('test')); + session[kSocket].emit('error', new Error('test')); })); server.listen(0, common.mustCall(() => { diff --git a/test/parallel/test-http2-socket-proxy.js b/test/parallel/test-http2-socket-proxy.js new file mode 100644 index 0000000000..60f3183779 --- /dev/null +++ b/test/parallel/test-http2-socket-proxy.js @@ -0,0 +1,88 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const h2 = require('http2'); +const net = require('net'); + +// Tests behaviour of the proxied socket on Http2Session + +const errMsg = { + code: 'ERR_HTTP2_NO_SOCKET_MANIPULATION', + type: Error, + message: 'HTTP/2 sockets should not be directly manipulated ' + + '(e.g. read and written)' +}; + +const server = h2.createServer(); + +server.on('stream', common.mustCall(function(stream, headers) { + const socket = stream.session.socket; + const session = stream.session; + + assert.ok(socket instanceof net.Socket); + + assert.strictEqual(socket.writable, true); + assert.strictEqual(socket.readable, true); + assert.strictEqual(typeof socket.address(), 'object'); + + socket.setTimeout(987); + assert.strictEqual(session._idleTimeout, 987); + + common.expectsError(() => socket.destroy, errMsg); + common.expectsError(() => socket.emit, errMsg); + common.expectsError(() => socket.end, errMsg); + common.expectsError(() => socket.pause, errMsg); + common.expectsError(() => socket.read, errMsg); + common.expectsError(() => socket.resume, errMsg); + common.expectsError(() => socket.write, errMsg); + + common.expectsError(() => (socket.destroy = undefined), errMsg); + common.expectsError(() => (socket.emit = undefined), errMsg); + common.expectsError(() => (socket.end = undefined), errMsg); + common.expectsError(() => (socket.pause = undefined), errMsg); + common.expectsError(() => (socket.read = undefined), errMsg); + common.expectsError(() => (socket.resume = undefined), errMsg); + common.expectsError(() => (socket.write = undefined), errMsg); + + assert.doesNotThrow(() => (socket.on = socket.on)); + assert.doesNotThrow(() => (socket.once = socket.once)); + + stream.respond(); + + socket.writable = 0; + socket.readable = 0; + assert.strictEqual(socket.writable, 0); + assert.strictEqual(socket.readable, 0); + + stream.session.destroy(); + + socket.setTimeout = undefined; + assert.strictEqual(session.setTimeout, undefined); + + stream.session.on('close', common.mustCall(() => { + assert.strictEqual(session.socket, undefined); + })); +})); + +server.listen(0, common.mustCall(function() { + const port = server.address().port; + const url = `http://localhost:${port}`; + const client = h2.connect(url, common.mustCall(() => { + const headers = { + ':path': '/', + ':method': 'GET', + ':scheme': 'http', + ':authority': `localhost:${port}` + }; + const request = client.request(headers); + request.on('end', common.mustCall(() => { + client.destroy(); + server.close(); + })); + request.end(); + request.resume(); + })); +}));