WIP net: enable emitclose in socket

Author: dnlup

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');
+    }
   }
 }
 

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;
 

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;
 

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

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

@@ -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,