WIP net: enable emitclose in socket

Author: dnlup

lib/internal/streams/destroy.js

@@ -151,12 +151,20 @@ 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 shouldEmitClose = (w && w.emitClose) || (r && r.emitClose);
+  const shouldPassErrorFlag = (w && w.errorFlagOnClose) ||
+    (r && r.errorFlagOnClose);
+
+  let flag;
+  if (shouldPassErrorFlag) {
+    flag = err ? true : false;
+  }
 
   if (w) {
     w.closeEmitted = true;
@@ -165,8 +173,8 @@ function emitCloseNT(self) {
     r.closeEmitted = true;
   }
 
-  if ((w && w.emitClose) || (r && r.emitClose)) {
-    self.emit('close');
+  if (shouldEmitClose) {
+    self.emit('close', flag);
   }
 }
 

lib/internal/streams/readable.js

@@ -134,6 +134,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;
 

lib/internal/streams/writable.js

@@ -180,6 +180,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;
 

lib/net.js

@@ -301,8 +301,8 @@ 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;
+  options.emitClose = true;
+  options.errorFlagOnClose = true;
   options.autoDestroy = true;
   // Handle strings directly.
   options.decodeStrings = false;
@@ -657,28 +657,20 @@ Socket.prototype._destroy = function(exception, cb) {
   if (this._handle) {
     if (this !== process.stderr)
       debug('close handle');
-    const isException = exception ? true : false;
+    // 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) {

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();
   }));

test/parallel/test-tls-socket-close.js

@@ -8,6 +8,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,