From b6990908dc5443a834938dc8dc6f0a05e43e74c7 Mon Sep 17 00:00:00 2001 From: Daniele Belardi Date: Mon, 28 Dec 2020 15:12:18 +0100 Subject: [PATCH] WIP net: enable emitclose in socket --- lib/internal/streams/destroy.js | 17 ++++++++++---- lib/internal/streams/readable.js | 4 ++++ lib/internal/streams/writable.js | 4 ++++ lib/net.js | 23 ++++--------------- ...ttp2-respond-with-file-connection-abort.js | 4 ++++ test/parallel/test-tls-socket-close.js | 5 ++++ 6 files changed, 34 insertions(+), 23 deletions(-) diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index 76df2aa0a34666..21d7561bf75b60 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -6,8 +6,8 @@ const { const { FunctionPrototypeCall, Symbol, + Boolean } = primordials; - const kDestroy = Symbol('kDestroy'); const kConstruct = Symbol('kConstruct'); @@ -155,12 +155,15 @@ function _destroy(self, err, cb) { function emitErrorCloseNT(self, err) { emitErrorNT(self, err); - emitCloseNT(self); + emitCloseNT(self, err); } -function emitCloseNT(self) { +function emitCloseNT(self, err) { const r = self._readableState; const w = self._writableState; + const emitClose = (w && w.emitClose) || (r && r.emitClose); + const shouldPassErrorFlag = (w && w.errorFlagOnClose) || + (r && r.errorFlagOnClose); if (w) { w.closeEmitted = true; @@ -169,8 +172,12 @@ function emitCloseNT(self) { r.closeEmitted = true; } - if ((w && w.emitClose) || (r && r.emitClose)) { - self.emit('close'); + if (emitClose) { + if (shouldPassErrorFlag) { + self.emit('close', Boolean(err)); + } else { + self.emit('close'); + } } } diff --git a/lib/internal/streams/readable.js b/lib/internal/streams/readable.js index 6667c919c086a7..4f9d989461395e 100644 --- a/lib/internal/streams/readable.js +++ b/lib/internal/streams/readable.js @@ -142,6 +142,10 @@ function ReadableState(options, stream, isDuplex) { // Should close be emitted on destroy. Defaults to true. this.emitClose = !options || options.emitClose !== false; + // Should a boolean flag be passed as argument when emitting close if an error + // occurred + this.errorFlagOnClose = options && options.errorFlagOnClose === true; + // Should .destroy() be called after 'end' (and potentially 'finish'). this.autoDestroy = !options || options.autoDestroy !== false; diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js index 2fe811236a27e8..5746735508045d 100644 --- a/lib/internal/streams/writable.js +++ b/lib/internal/streams/writable.js @@ -186,6 +186,10 @@ function WritableState(options, stream, isDuplex) { // Should close be emitted on destroy. Defaults to true. this.emitClose = !options || options.emitClose !== false; + // Should a boolean flag be passed as argument when emitting close if an error + // occurred + this.errorFlagOnClose = options && options.errorFlagOnClose === true; + // Should .destroy() be called after 'finish' (and potentially 'end'). this.autoDestroy = !options || options.autoDestroy !== false; diff --git a/lib/net.js b/lib/net.js index 94aae1fa9f736d..dceabb486b8ed1 100644 --- a/lib/net.js +++ b/lib/net.js @@ -299,8 +299,9 @@ function Socket(options) { // Default to *not* allowing half open sockets. options.allowHalfOpen = Boolean(options.allowHalfOpen); - // For backwards compat do not emit close on destroy. - options.emitClose = false; + // For backward compatibility, pass a flag when emitting close for + // comunicating that an error occurred. + options.errorFlagOnClose = true; options.autoDestroy = true; // Handle strings directly. options.decodeStrings = false; @@ -655,28 +656,20 @@ Socket.prototype._destroy = function(exception, cb) { if (this._handle) { if (this !== process.stderr) debug('close handle'); - const isException = exception ? true : false; + // `bytesRead` and `kBytesWritten` should be accessible after `.destroy()` this[kBytesRead] = this._handle.bytesRead; this[kBytesWritten] = this._handle.bytesWritten; this._handle.close(() => { debug('emit close'); - if (this._writableState) { - this._writableState.closed = true; - } - if (this._readableState) { - this._readableState.closed = true; - } - this.emit('close', isException); + cb(exception); }); this._handle.onread = noop; this._handle = null; this._sockname = null; - cb(exception); } else { cb(exception); - process.nextTick(emitCloseNT, this); } if (this._server) { @@ -1655,12 +1648,6 @@ Server.prototype._emitCloseIfDrained = function() { function emitCloseNT(self) { debug('SERVER: emit close'); - if (self._writableState) { - self._writableState.closed = true; - } - if (self._readableState) { - self._readableState.closed = true; - } self.emit('close'); } diff --git a/test/parallel/test-http2-respond-with-file-connection-abort.js b/test/parallel/test-http2-respond-with-file-connection-abort.js index ed200e5b391fe8..7fbc3c8cbad534 100644 --- a/test/parallel/test-http2-respond-with-file-connection-abort.js +++ b/test/parallel/test-http2-respond-with-file-connection-abort.js @@ -25,6 +25,10 @@ server.listen(0, common.mustCall(() => { req.on('response', common.mustCall(() => {})); req.once('data', common.mustCall(() => { + // TODO: is this test needed? + // It errors with ERR_HTTP2_NO_SOCKET_MANIPULATION because we are + // calling destroy on the Proxy(ed) socket of the Http2ClientSession + // which causes `emit` to becalled and the error to be thrown. net.Socket.prototype.destroy.call(client.socket); server.close(); })); diff --git a/test/parallel/test-tls-socket-close.js b/test/parallel/test-tls-socket-close.js index 87355cf8d7bd2d..851c9a2615e689 100644 --- a/test/parallel/test-tls-socket-close.js +++ b/test/parallel/test-tls-socket-close.js @@ -1,4 +1,5 @@ 'use strict'; + const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); @@ -8,6 +9,10 @@ const tls = require('tls'); const net = require('net'); const fixtures = require('../common/fixtures'); +// TODO: is this test needed? +// It fails with a timeout error because the `error` event is never emitted. +// `write` doesn't error, is that a good thing? + // Regression test for https://github.com/nodejs/node/issues/8074 // // This test has a dependency on the order in which the TCP connection is made,