fix test-net-server-max-connections.js (#20034)

Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Co-authored-by: nektro <5464072+nektro@users.noreply.github.com>
Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>

src/js/node/net.ts

@@ -28,7 +28,8 @@ let dns: typeof import("node:dns");
 const normalizedArgsSymbol = Symbol("normalizedArgs");
 const { ExceptionWithHostPort } = require("internal/shared");
 import type { Socket, SocketHandler, SocketListener } from "bun";
-import type { ServerOpts } from "node:net";
+import type { Server as NetServer, Socket as NetSocket, ServerOpts } from "node:net";
+import type { TLSSocket } from "node:tls";
 const { kTimeout, getTimerDuration } = require("internal/timers");
 const { validateFunction, validateNumber, validateAbortSignal, validatePort, validateBoolean, validateInt32 } = require("internal/validators"); // prettier-ignore
 const { NodeAggregateError, ErrnoException } = require("internal/shared");
@@ -325,7 +326,7 @@ const SocketEmitEndNT = (self, _err?) => {
   // }
 };
 
-const ServerHandlers: SocketHandler = {
+const ServerHandlers: SocketHandler<NetSocket> = {
   data(socket, buffer) {
     const { data: self } = socket;
     if (!self) return;
@@ -336,10 +337,10 @@ const ServerHandlers: SocketHandler = {
     }
   },
   close(socket, err) {
+    $debug("Bun.Server close");
     const data = this.data;
     if (!data) return;
 
-    data.server._connections--;
     {
       if (!data[kclosed]) {
         data[kclosed] = true;
@@ -350,20 +351,18 @@ const ServerHandlers: SocketHandler = {
         socket[owner_symbol] = null;
       }
     }
-
-    data.server._emitCloseIfDrained();
   },
   end(socket) {
     SocketHandlers.end(socket);
   },
   open(socket) {
-    const self = this.data;
+    $debug("Bun.Server open");
+    const self = socket.data as any as NetServer;
     socket[kServerSocket] = self._handle;
     const options = self[bunSocketServerOptions];
     const { pauseOnConnect, connectionListener, [kSocketClass]: SClass, requestCert, rejectUnauthorized } = options;
-    const _socket = new SClass({});
+    const _socket = new SClass({}) as NetSocket | TLSSocket;
     _socket.isServer = true;
-    _socket.server = self;
     _socket._requestCert = requestCert;
     _socket._rejectUnauthorized = rejectUnauthorized;
 
@@ -396,7 +395,6 @@ const ServerHandlers: SocketHandler = {
       };
 
       socket.end();
-
       self.emit("drop", data);
       return;
     }
@@ -405,6 +403,7 @@ const ServerHandlers: SocketHandler = {
     const isTLS = typeof bunTLS === "function";
 
     self._connections++;
+    _socket.server = self;
 
     if (pauseOnConnect) {
       _socket.pause();
@@ -423,7 +422,7 @@ const ServerHandlers: SocketHandler = {
     }
   },
   handshake(socket, success, verifyError) {
-    const { data: self } = socket;
+    const self = socket.data;
     if (!success && verifyError?.code === "ECONNRESET") {
       const err = new ConnResetException("socket hang up");
       self.emit("_tlsError", err);
@@ -437,7 +436,7 @@ const ServerHandlers: SocketHandler = {
     self.secureConnecting = false;
     self._secureEstablished = !!success;
     self.servername = socket.getServername();
-    const server = self.server;
+    const server = self.server!;
     self.alpnProtocol = socket.alpnProtocol;
     if (self._requestCert || self._rejectUnauthorized) {
       if (verifyError) {
@@ -510,9 +509,8 @@ const ServerHandlers: SocketHandler = {
   binaryType: "buffer",
 } as const;
 
-type SocketHandleData = NonNullable<import("node:net").Socket["_handle"]>["data"];
 // TODO: SocketHandlers2 is a bad name but its temporary. reworking the Server in a followup PR
-const SocketHandlers2: SocketHandler<SocketHandleData> = {
+const SocketHandlers2: SocketHandler<NonNullable<import("node:net").Socket["_handle"]>["data"]> = {
   open(socket) {
     $debug("Bun.Socket open");
     let { self, req } = socket.data;
@@ -1165,6 +1163,14 @@ Socket.prototype._destroy = function _destroy(err, callback) {
     callback(err);
     process.nextTick(emitCloseNT, this, false);
   }
+
+  if (this.server) {
+    $debug("has server");
+    this.server._connections--;
+    if (this.server._emitCloseIfDrained) {
+      this.server._emitCloseIfDrained();
+    }
+  }
 };
 
 Socket.prototype._final = function _final(callback) {
@@ -2378,6 +2384,7 @@ Server.prototype[kRealListen] = function (
       ipv6Only: ipv6Only || this[bunSocketServerOptions]?.ipv6Only || false,
       exclusive: exclusive || this[bunSocketServerOptions]?.exclusive || false,
       socket: ServerHandlers,
+      data: this,
     });
   } else if (fd != null) {
     this._handle = Bun.listen({
@@ -2389,6 +2396,7 @@ Server.prototype[kRealListen] = function (
       ipv6Only: ipv6Only || this[bunSocketServerOptions]?.ipv6Only || false,
       exclusive: exclusive || this[bunSocketServerOptions]?.exclusive || false,
       socket: ServerHandlers,
+      data: this,
     });
   } else {
     this._handle = Bun.listen({
@@ -2400,12 +2408,10 @@ Server.prototype[kRealListen] = function (
       ipv6Only: ipv6Only || this[bunSocketServerOptions]?.ipv6Only || false,
       exclusive: exclusive || this[bunSocketServerOptions]?.exclusive || false,
       socket: ServerHandlers,
+      data: this,
     });
   }
 
-  //make this instance available on handlers
-  this._handle.data = this;
-
   const addr = this.address();
   if (addr && typeof addr === "object") {
     const familyLast = String(addr.family).slice(-1);

src/js/private.d.ts

@@ -224,10 +224,17 @@ declare function $newZigFunction<T = (...args: any) => any>(
 declare function $bindgenFn<T = (...args: any) => any>(filename: string, symbol: string): T;
 // NOTE: $debug, $assert, and $isPromiseFulfilled omitted
 
+import "node:net";
 declare module "node:net" {
   export function _normalizeArgs(args: any[]): unknown[];
 
   interface Socket {
     _handle: Bun.Socket<{ self: Socket; req?: object }> | null;
+    server: Server | null;
+  }
+
+  interface Server {
+    _handle: Bun.SocketListener<Socket> | null;
+    _connections: number;
   }
 }

test/js/node/test/parallel/test-net-server-max-connections-close-makes-more-available.js

@@ -0,0 +1,85 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+
+const net = require('net');
+
+// Sets the server's maxConnections property to 1.
+// Open 2 connections (connection 0 and connection 1).
+// Connection 0  should be accepted.
+// Connection 1 should be rejected.
+// Closes connection 0.
+// Open 2 more connections (connection 2 and 3).
+// Connection 2 should be accepted.
+// Connection 3 should be rejected.
+
+const connections = [];
+const received = [];
+const sent = [];
+
+function createConnection(index) {
+  console.error(`creating connection ${index}`);
+
+  return new Promise(function(resolve, reject) {
+    const connection = net.createConnection(server.address().port, function() {
+      const msg = String(index);
+      console.error(`sending message: ${msg}`);
+      this.write(msg);
+      sent.push(msg);
+    });
+
+    connection.on('error', function(err) {
+      assert.strictEqual(err.code, 'ECONNRESET');
+      resolve();
+    });
+
+    connection.on('data', function(e) {
+      console.error(`connection ${index} received response`);
+      resolve();
+    });
+
+    connection.on('end', function() {
+      console.error(`ending ${index}`);
+      resolve();
+    });
+
+    connections[index] = connection;
+  });
+}
+
+function closeConnection(index) {
+  console.error(`closing connection ${index}`);
+  return new Promise(function(resolve, reject) {
+    connections[index].on('end', function() {
+      resolve();
+    });
+    connections[index].end();
+  });
+}
+
+const server = net.createServer(function(socket) {
+  socket.on('data', function(data) {
+    console.error(`received message: ${data}`);
+    received.push(String(data));
+    socket.write('acknowledged');
+  });
+});
+
+server.maxConnections = 1;
+
+server.listen(0, function() {
+  createConnection(0)
+    .then(createConnection.bind(null, 1))
+    .then(closeConnection.bind(null, 0))
+    .then(createConnection.bind(null, 2))
+    .then(createConnection.bind(null, 3))
+    .then(server.close.bind(server))
+    .then(closeConnection.bind(null, 2));
+});
+
+process.on('exit', function() {
+  // Confirm that all connections tried to send data...
+  assert.deepStrictEqual(sent, ['0', '1', '2', '3']);
+  // ...but that only connections 0 and 2 were successful.
+  assert.deepStrictEqual(received, ['0', '2']);
+});

test/js/node/test/parallel/test-net-server-max-connections.js

@@ -0,0 +1,107 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+const net = require('net');
+
+// This test creates 20 connections to a server and sets the server's
+// maxConnections property to 10. The first 10 connections make it through
+// and the last 10 connections are rejected.
+
+const N = 20;
+let closes = 0;
+const waits = [];
+
+const server = net.createServer(common.mustCall(function(connection) {
+  connection.write('hello');
+  waits.push(function() { connection.end(); });
+}, N / 2));
+
+server.listen(0, function() {
+  makeConnection(0);
+});
+
+server.maxConnections = N / 2;
+
+
+function makeConnection(index) {
+  const c = net.createConnection(server.address().port);
+  let gotData = false;
+
+  c.on('connect', function() {
+    if (index + 1 < N) {
+      makeConnection(index + 1);
+    }
+
+    c.on('close', function() {
+      // console.error(`closed ${index}`);
+      closes++;
+
+      if (closes < N / 2) {
+        assert.ok(
+          server.maxConnections <= index,
+          `${index} should not have been one of the first closed connections`
+        );
+      }
+
+      if (closes === N / 2) {
+        let cb;
+        console.error('calling wait callback.');
+        while ((cb = waits.shift()) !== undefined) {
+          cb();
+        }
+        server.close();
+      }
+
+      if (index < server.maxConnections) {
+        assert.strictEqual(gotData, true,
+                           `${index} didn't get data, but should have`);
+      } else {
+        assert.strictEqual(gotData, false,
+                           `${index} got data, but shouldn't have`);
+      }
+    });
+  });
+
+  c.on('end', function() { c.end(); });
+
+  c.on('data', function(b) {
+    gotData = true;
+    assert.ok(b.length > 0);
+  });
+
+  c.on('error', function(e) {
+    // Retry if SmartOS and ECONNREFUSED. See
+    // https://github.com/nodejs/node/issues/2663.
+    if (common.isSunOS && (e.code === 'ECONNREFUSED')) {
+      c.connect(server.address().port);
+    }
+    console.error(`error ${index}: ${e}`);
+  });
+}
+
+
+process.on('exit', function() {
+  assert.strictEqual(closes, N);
+});