Issues with net.Server when using a pipe

[joaocgreis]

• Version: current master
• Platform: Unix, at least Ubuntu 18.04. The callback issue also affects Windows.
• Subsystem: net

Discovered while working on #28858. I investigated this for a while but wasn't able to solve the issue. Opening this to keep track, or to get an explanation if someone knows what's going on.

There seem to be two (possibly related) issues when using a pipe with net.Server:

The pipe file descriptor is not closed (or one of them? there seems to be more than one in some cases).

Patch to reproduce:

diff --git a/test/parallel/test-child-process-server-close.js b/test/parallel/test-child-process-server-close.js
index d70926f2e8..180f51499d 100644
--- a/test/parallel/test-child-process-server-close.js
+++ b/test/parallel/test-child-process-server-close.js
@@ -7,6 +7,13 @@ const net = require('net');
 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();

+const { execSync } = require('child_process');
+const lsof = `lsof -p ${process.pid} | grep -e ^COMMAND -e tmp`;
+process.on('exit', () => {
+  const openFiles = execSync(lsof, { encoding: 'utf8' });
+  console.error(`Open files (${lsof}):\n${openFiles}`);
+});
+
 const server = net.createServer((conn) => {
   spawn(process.execPath, ['-v'], {
     stdio: ['ignore', conn, 'ignore']
diff --git a/test/parallel/test-tls-wrap-econnreset-pipe.js b/test/parallel/test-tls-wrap-econnreset-pipe.js
index b400e35d41..5d1070b63c 100644
--- a/test/parallel/test-tls-wrap-econnreset-pipe.js
+++ b/test/parallel/test-tls-wrap-econnreset-pipe.js
@@ -11,6 +11,13 @@ const net = require('net');
 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();

+const { execSync } = require('child_process');
+const lsof = `lsof -p ${process.pid} | grep -e ^COMMAND -e tmp`;
+process.on('exit', () => {
+  const openFiles = execSync(lsof, { encoding: 'utf8' });
+  console.error(`Open files (${lsof}):\n${openFiles}`);
+});
+
 const server = net.createServer((c) => {
   c.end();
 }).listen(common.PIPE, common.mustCall(() => {

The callback of `server.close()` is never called.

Patch to reproduce:

diff --git a/test/parallel/test-child-process-server-close.js b/test/parallel/test-child-process-server-close.js
index d70926f2e8..b63245b160 100644
--- a/test/parallel/test-child-process-server-close.js
+++ b/test/parallel/test-child-process-server-close.js
@@ -7,6 +7,9 @@ const net = require('net');
 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();

+const runsOk = common.mustCall(function runsOk() {});
+const neverRuns = common.mustCall(function neverRuns() {});
+
 const server = net.createServer((conn) => {
   spawn(process.execPath, ['-v'], {
     stdio: ['ignore', conn, 'ignore']
@@ -17,7 +20,8 @@ const server = net.createServer((conn) => {
   const client = net.connect(common.PIPE, common.mustCall());
   client.on('data', () => {
     client.end(() => {
-      server.close();
+      runsOk();
+      server.close(neverRuns);
     });
   });
 });
diff --git a/test/parallel/test-tls-wrap-econnreset-pipe.js b/test/parallel/test-tls-wrap-econnreset-pipe.js
index b400e35d41..e02a2f08b6 100644
--- a/test/parallel/test-tls-wrap-econnreset-pipe.js
+++ b/test/parallel/test-tls-wrap-econnreset-pipe.js
@@ -11,6 +11,9 @@ const net = require('net');
 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();

+const runsOk = common.mustCall(function runsOk() {});
+const neverRuns = common.mustCall(function neverRuns() {});
+
 const server = net.createServer((c) => {
   c.end();
 }).listen(common.PIPE, common.mustCall(() => {
@@ -21,6 +24,7 @@ const server = net.createServer((c) => {
       assert.strictEqual(e.port, undefined);
       assert.strictEqual(e.host, undefined);
       assert.strictEqual(e.localAddress, undefined);
-      server.close();
+      runsOk();
+      server.close(neverRuns);
     }));
 }));

This doesn't happen with all the tests that use pipes, but only with parallel/test-child-process-server-close and parallel/test-tls-wrap-econnreset-pipe. The first one passes the pipe as the stdout of a child process and the second triggers an error in the server, so this issue may be related to that and actually expected.

cc @bnoordhuis, @indutny, @nodejs/streams

[bnoordhuis]

The callback of server.close() is never called.

node/lib/net.js

Lines 1557 to 1561 in 83495e7

	if (this._handle || this._connections) {
	debug('SERVER handle? %j connections? %d',
	!!this._handle, this._connections);
	return;
	}

The server.close() callback doesn't execute because server._connections === 1. This patch fixes it:

diff --git a/test/parallel/test-child-process-server-close.js b/test/parallel/test-child-process-server-close.js
index d70926f2e8..6e49b8102b 100644
--- a/test/parallel/test-child-process-server-close.js
+++ b/test/parallel/test-child-process-server-close.js
@@ -11,13 +11,9 @@ const server = net.createServer((conn) => {
   spawn(process.execPath, ['-v'], {
     stdio: ['ignore', conn, 'ignore']
   }).on('close', common.mustCall(() => {
-    conn.end();
+    conn.destroy(null, () => server.close());
   }));
 }).listen(common.PIPE, () => {
   const client = net.connect(common.PIPE, common.mustCall());
-  client.on('data', () => {
-    client.end(() => {
-      server.close();
-    });
-  });
+  client.on('data', () => client.end());
 });

Calling server.close() after destroying the client connection is arguably always wrong because the server might not have processed the client-going-away event yet. It needs to act on the peer, the conn object in this test.

[bnoordhuis]

This issue has been open for 3 years with no movement. No one else chimed in and it's not even clear to me which node version it applied to, so... I'm going to close it.