net: use _final instead of on('finish')

addaleax · rvagg · commit 3d38bab64e36 · 2018-08-16T11:38:51.000+10:00
Shutting down the connection is what `_final` is there for. PR-URL: #18608 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js
@@ -55,6 +55,8 @@ function undestroy() {
     this._writableState.destroyed = false;
     this._writableState.ended = false;
     this._writableState.ending = false;
+    this._writableState.finalCalled = false;
+    this._writableState.prefinished = false;
     this._writableState.finished = false;
     this._writableState.errorEmitted = false;
   }
diff --git a/lib/net.js b/lib/net.js
@@ -229,7 +229,6 @@ function Socket(options) {
   }
 
   // shut down the socket when we're finished with it.
-  this.on('finish', onSocketFinish);
   this.on('_socketEnd', onSocketEnd);
 
   initSocketHandle(this);
@@ -273,39 +272,42 @@ Socket.prototype._unrefTimer = function _unrefTimer() {
 
 function shutdownSocket(self, callback) {
   var req = new ShutdownWrap();
-  req.oncomplete = callback;
+  req.oncomplete = afterShutdown;
   req.handle = self._handle;
+  req.callback = callback;
   return self._handle.shutdown(req);
 }
 
 // the user has called .end(), and all the bytes have been
 // sent out to the other side.
-function onSocketFinish() {
-  // If still connecting - defer handling 'finish' until 'connect' will happen
+Socket.prototype._final = function(cb) {
+  // If still connecting - defer handling `_final` until 'connect' will happen
   if (this.connecting) {
-    debug('osF: not yet connected');
-    return this.once('connect', onSocketFinish);
+    debug('_final: not yet connected');
+    return this.once('connect', () => this._final(cb));
   }
 
-  debug('onSocketFinish');
   if (!this.readable || this._readableState.ended) {
-    debug('oSF: ended, destroy', this._readableState);
+    debug('_final: ended, destroy', this._readableState);
+    cb();
     return this.destroy();
   }
 
-  debug('oSF: not ended, call shutdown()');
+  debug('_final: not ended, call shutdown()');
 
   // otherwise, just shutdown, or destroy() if not possible
-  if (!this._handle || !this._handle.shutdown)
+  if (!this._handle || !this._handle.shutdown) {
+    cb();
     return this.destroy();
+  }
 
   var err = defaultTriggerAsyncIdScope(
-    this[async_id_symbol], shutdownSocket, this, afterShutdown
+    this[async_id_symbol], shutdownSocket, this, cb
   );
 
   if (err)
     return this.destroy(errnoException(err, 'shutdown'));
-}
+};
 
 
 function afterShutdown(status, handle, req) {
@@ -314,6 +316,8 @@ function afterShutdown(status, handle, req) {
   debug('afterShutdown destroyed=%j', self.destroyed,
         self._readableState);
 
+  this.callback();
+
   // callback may come after call to destroy.
   if (self.destroyed)
     return;
diff --git a/test/async-hooks/test-shutdownwrap.js b/test/async-hooks/test-shutdownwrap.js
@@ -24,11 +24,13 @@ let endedConnection = false;
 function onconnection(c) {
   assert.strictEqual(hooks.activitiesOfTypes('SHUTDOWNWRAP').length, 0);
   c.end();
-  endedConnection = true;
-  const as = hooks.activitiesOfTypes('SHUTDOWNWRAP');
-  assert.strictEqual(as.length, 1);
-  checkInvocations(as[0], { init: 1 }, 'after ending client connection');
-  this.close(onserverClosed);
+  process.nextTick(() => {
+    endedConnection = true;
+    const as = hooks.activitiesOfTypes('SHUTDOWNWRAP');
+    assert.strictEqual(as.length, 1);
+    checkInvocations(as[0], { init: 1 }, 'after ending client connection');
+    this.close(onserverClosed);
+  });
 }
 
 function onconnected() {
diff --git a/test/parallel/test-stream-writable-destroy.js b/test/parallel/test-stream-writable-destroy.js
@@ -185,3 +185,17 @@ const { inherits } = require('util');
     assert.strictEqual(expected, err);
   }));
 }
+
+{
+  // Checks that `._undestroy()` restores the state so that `final` will be
+  // called again.
+  const write = new Writable({
+    write: common.mustNotCall(),
+    final: common.mustCall((cb) => cb(), 2)
+  });
+
+  write.end();
+  write.destroy();
+  write._undestroy();
+  write.end();
+}

Original file line number	Diff line number	Diff line change
`@@ -55,6 +55,8 @@ function undestroy() {`
`55`	`55`	`this._writableState.destroyed = false;`
`56`	`56`	`this._writableState.ended = false;`
`57`	`57`	`this._writableState.ending = false;`
	`58`	`+ this._writableState.finalCalled = false;`
	`59`	`+ this._writableState.prefinished = false;`
`58`	`60`	`this._writableState.finished = false;`
`59`	`61`	`this._writableState.errorEmitted = false;`
`60`	`62`	`}`