From fbac8e870ba990f5b45c1feae3802f95591541c5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Nov 2018 21:09:42 +0100 Subject: [PATCH 1/3] tls: do not rely on 'drain' handlers in StreamWrap `'drain'` event handlers may not be invoked if the stream is currently finishing. Instead, use the fact that we know when writes are active or not, and invoke the delayed shutdown handler from our own after-write callback. --- lib/internal/wrap_js_stream.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/internal/wrap_js_stream.js b/lib/internal/wrap_js_stream.js index 7ca7ff8bf49d25..cf8f45aa4505ff 100644 --- a/lib/internal/wrap_js_stream.js +++ b/lib/internal/wrap_js_stream.js @@ -11,6 +11,7 @@ const { ERR_STREAM_WRAP } = require('internal/errors').codes; const kCurrentWriteRequest = Symbol('kCurrentWriteRequest'); const kCurrentShutdownRequest = Symbol('kCurrentShutdownRequest'); +const kPendingShutdownRequest = Symbol('kPendingShutdownRequest'); function isClosing() { return this[owner_symbol].isClosing(); } function onreadstart() { return this[owner_symbol].readStart(); } @@ -79,6 +80,7 @@ class JSStreamWrap extends Socket { this.stream = stream; this[kCurrentWriteRequest] = null; this[kCurrentShutdownRequest] = null; + this[kPendingShutdownRequest] = null; this.readable = stream.readable; this.writable = stream.writable; @@ -115,8 +117,10 @@ class JSStreamWrap extends Socket { // Working around that on the native side is not quite trivial (yet?), // so for now that is supported here. - if (this[kCurrentWriteRequest] !== null) - return this.once('drain', () => this.doShutdown(req)); + if (this[kCurrentWriteRequest] !== null) { + this[kPendingShutdownRequest] = req; + return 0; + } assert.strictEqual(this[kCurrentWriteRequest], null); assert.strictEqual(this[kCurrentShutdownRequest], null); this[kCurrentShutdownRequest] = req; @@ -189,6 +193,11 @@ class JSStreamWrap extends Socket { this[kCurrentWriteRequest] = null; handle.finishWrite(req, errCode); + if (this[kPendingShutdownRequest]) { + const req = this[kPendingShutdownRequest]; + this[kPendingShutdownRequest] = null; + this.doShutdown(req); + } } doClose(cb) { From c58bced696227902a182de88b128963eb600c108 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Nov 2018 21:11:17 +0100 Subject: [PATCH 2/3] tls: destroy TLS socket if StreamWrap is destroyed Previously, there was no mechanism in place that would have destroyed the TLS socket once the underlying socket had been closed. --- lib/_tls_wrap.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index f0d86f3d870f09..fe3e6edd152d7e 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -308,10 +308,12 @@ function TLSSocket(socket, opts) { // Wrap plain JS Stream into StreamWrap var wrap; - if ((socket instanceof net.Socket && socket._handle) || !socket) + if ((socket instanceof net.Socket && socket._handle) || !socket) { wrap = socket; - else + } else { wrap = new StreamWrap(socket); + wrap.once('close', () => this.destroy()); + } // Just a documented property to make secure sockets // distinguishable from regular ones. From 4cae5b8ec616f57e1618bc7160ff4fb41c116255 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Nov 2018 21:03:46 +0100 Subject: [PATCH 3/3] Revert "net: partially revert "simplify Socket.prototype._final"" This reverts commit ac1f56c76a5d1a8ebcb2421d5c629e51df1ac48c. Refs: https://github.com/nodejs/node/pull/24288 Refs: https://github.com/nodejs/node/pull/24075 --- lib/net.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/net.js b/lib/net.js index 99dc76c8f3baf5..5d332d9e1d30d7 100644 --- a/lib/net.js +++ b/lib/net.js @@ -345,12 +345,6 @@ Socket.prototype._final = function(cb) { return this.once('connect', () => this._final(cb)); } - // TODO(addaleax): This should not be necessary. - if (!this.readable || this._readableState.ended) { - cb(); - return this.destroy(); - } - if (!this._handle) return cb();