net: wait for shutdown to complete before closing

When not allowing half open, handle.close would be
invoked before shutdown has been called and
completed causing a potential data race.

Fixes: https://github.com/nodejs/node/issues/32486#issuecomment-604072559

PR-URL: https://github.com/nodejs/node/pull/32491
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

lib/internal/stream_base_commons.js

@@ -213,6 +213,16 @@ function onStreamRead(arrayBuffer) {
     if (stream[kMaybeDestroy])
       stream.on('end', stream[kMaybeDestroy]);
 
+    // TODO(ronag): Without this `readStop`, `onStreamRead`
+    // will be called once more (i.e. after Readable.ended)
+    // on Windows causing a ECONNRESET, failing the
+    // test-https-truncate test.
+    if (handle.readStop) {
+      const err = handle.readStop();
+      if (err)
+        return stream.destroy(errnoException(err, 'read'));
+    }
+
     // Push a null to signal the end of data.
     // Do it before `maybeDestroy` for correct order of events:
     // `end` -> `close`

lib/net.js

@@ -630,9 +630,9 @@ function onReadableStreamEnd() {
     this.write = writeAfterFIN;
     if (this.writable)
       this.end();
-  }
-
-  if (!this.destroyed && !this.writable && !this.writableLength)
+    else if (!this.writableLength)
+      this.destroy();
+  } else if (!this.destroyed && !this.writable && !this.writableLength)
     this.destroy();
 }
 

test/async-hooks/test-graph.tls-write.js

@@ -65,9 +65,7 @@ function onexit() {
       { type: 'TCPCONNECTWRAP',
         id: 'tcpconnect:1', triggerAsyncId: 'tcp:1' },
       { type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' },
-      { type: 'TLSWRAP', id: 'tls:2', triggerAsyncId: 'tcpserver:1' },
-      { type: 'Immediate', id: 'immediate:1', triggerAsyncId: 'tcp:2' },
-      { type: 'Immediate', id: 'immediate:2', triggerAsyncId: 'tcp:1' },
+      { type: 'TLSWRAP', id: 'tls:2', triggerAsyncId: 'tcpserver:1' }
     ]
   );
 }

test/parallel/test-net-allow-half-open.js

@@ -0,0 +1,25 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const net = require('net');
+
+const server = net.createServer(common.mustCall((socket) => {
+  socket.end(Buffer.alloc(1024));
+})).listen(0, common.mustCall(() => {
+  const socket = net.connect(server.address().port);
+  assert.strictEqual(socket.allowHalfOpen, false);
+  socket.resume();
+  socket.on('end', common.mustCall(() => {
+    process.nextTick(() => {
+      // Ensure socket is not destroyed straight away
+      // without proper shutdown.
+      assert(!socket.destroyed);
+      server.close();
+    });
+  }));
+  socket.on('finish', common.mustCall(() => {
+    assert(!socket.destroyed);
+  }));
+  socket.on('close', common.mustCall());
+}));