Don't call the onTimeout handler if the socket is closed, shutdown, or detached

src/bun.js/api/bun/socket.zig

@@ -1197,7 +1197,6 @@ fn NewSocket(comptime ssl: bool) type {
         this_value: JSC.JSValue = .zero,
         poll_ref: Async.KeepAlive = Async.KeepAlive.init(),
         is_active: bool = false,
-        last_4: [4]u8 = .{ 0, 0, 0, 0 },
         authorized: bool = false,
         connection: ?Listener.UnixOrHost = null,
         protos: ?[]const u8,
@@ -1297,7 +1296,7 @@ fn NewSocket(comptime ssl: bool) type {
         ) void {
             JSC.markBinding(@src());
             log("onTimeout", .{});
-            if (this.detached) return;
+            if (this.detached or this.socket.isShutdown() or this.socket.isClosed()) return;
 
             const handlers = this.handlers;
             const callback = handlers.onTimeout;
@@ -1758,12 +1757,15 @@ fn NewSocket(comptime ssl: bool) type {
                 return .zero;
             }
             const t = args.ptr[0].coerce(i32, globalObject);
+            if (globalObject.hasException())
+                return .zero;
+
             if (t < 0) {
                 globalObject.throw("Timeout must be a positive integer", .{});
                 return .zero;
             }
 
-            this.socket.setTimeout(@as(c_uint, @intCast(t)));
+            this.socket.setTimeout(@intCast(t));
 
             return JSValue.jsUndefined();
         }

src/js/node/net.ts

@@ -32,6 +32,17 @@ var IPv4Reg;
 const v6Seg = "(?:[0-9a-fA-F]{1,4})";
 var IPv6Reg;
 
+function convertTimeout(timeout) {
+  // uSockets timeouts are in seconds...
+  // Node.js timeouts are in milliseconds...
+  if (typeof timeout === "number") {
+    // Math.ceil() because if you set a timeout to 1 you don't want to disable it.
+    return Math.ceil(timeout / 1000);
+  }
+
+  return 0;
+}
+
 function isIPv4(s) {
   return (IPv4Reg ??= new RegExp(`^${v4Str}$`)).test(s);
 }
@@ -124,8 +135,8 @@ const Socket = (function (InternalSocket) {
       open(socket) {
         const self = socket.data;
         if (!self) return;
-
-        socket.timeout(self.timeout);
+        const timeout = self.timeout;
+        socket.timeout(convertTimeout(timeout));
         if (self.#unrefOnConnected) socket.unref();
         self[bunSocketInternal] = socket;
         self.connecting = false;
@@ -182,7 +193,7 @@ const Socket = (function (InternalSocket) {
         const self = socket.data;
         if (!self) return;
 
-        self.emit("timeout", self);
+        self._onTimeout();
       },
       binaryType: "buffer",
     };
@@ -399,10 +410,23 @@ const Socket = (function (InternalSocket) {
       return this.writableLength;
     }
 
+    // Node implements `_onTimeout()`:
+    // https://github.com/nodejs/node/blob/2eff28fb7a93d3f672f80b582f664a7c701569fb/lib/net.js#L577-L593
+    _onTimeout() {
+      if (
+        this.timeout &&
+        // node suppresses the timeout if there is an active write in progress
+        !this.#writeCallback
+      ) {
+        this.emit("timeout");
+      }
+    }
+
     #attach(port, socket) {
       this.remotePort = port;
       socket.data = this;
-      socket.timeout(this.timeout);
+      const timeout = this.timeout;
+      socket.timeout(convertTimeout(timeout));
       if (this.#unrefOnConnected) socket.unref();
       this[bunSocketInternal] = socket;
       this.connecting = false;
@@ -517,8 +541,8 @@ const Socket = (function (InternalSocket) {
         this._secureEstablished = false;
         this._securePending = true;
 
-        if (connectListener) this.on("secureConnect", connectListener);
-      } else if (connectListener) this.on("connect", connectListener);
+        if (connectListener) this.once("secureConnect", connectListener);
+      } else if (connectListener) this.once("connect", connectListener);
 
       // start using existing connection
       try {
@@ -537,7 +561,7 @@ const Socket = (function (InternalSocket) {
               const [raw, tls] = result;
               // replace socket
               connection[bunSocketInternal] = raw;
-              raw.timeout(raw.timeout);
+              raw.timeout(convertTimeout(this.timeout));
               this.once("end", this.#closeRawConnection);
               raw.connecting = false;
               this[bunSocketInternal] = tls;
@@ -563,7 +587,7 @@ const Socket = (function (InternalSocket) {
                 const [raw, tls] = result;
                 // replace socket
                 connection[bunSocketInternal] = raw;
-                raw.timeout(raw.timeout);
+                raw.timeout(convertTimeout(this.timeout));
                 this.once("end", this.#closeRawConnection);
                 raw.connecting = false;
                 this[bunSocketInternal] = tls;
@@ -683,9 +707,14 @@ const Socket = (function (InternalSocket) {
     }
 
     setTimeout(timeout, callback) {
-      this[bunSocketInternal]?.timeout(timeout);
+      if (this.destroyed) return this;
+
+      const handle = this[bunSocketInternal];
+      if (handle) {
+        handle.timeout(convertTimeout(timeout));
+      }
       this.timeout = timeout;
-      if (callback) this.once("timeout", callback);
+      if (callback && $isCallable(callback)) this.once("timeout", callback);
       return this;
     }
 

test/js/node/net/node-net.test.ts

@@ -490,3 +490,52 @@ it("socket should keep process alive if unref is not called", async () => {
   });
   expect(await process.exited).toBe(1);
 });
+
+it("onTimeout shouldn't happen if there is a write in progress", async () => {
+  let otherSocket;
+  using listener = Bun.listen({
+    port: 0,
+    hostname: "localhost",
+    socket: {
+      open(socket) {
+        console.log("Open");
+        otherSocket = socket;
+        socket.write("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n");
+      },
+      data() {},
+      close() {},
+    },
+  });
+
+  const socket = createConnection(listener.port, "localhost");
+  // 1. Verify the timeout happens.
+  let { promise, resolve } = Promise.withResolvers();
+  socket.setTimeout(1, () => {
+    resolve();
+  });
+  socket.write("abc");
+  await promise;
+
+  {
+    let hasTimeout = false;
+    ({ promise, resolve } = Promise.withResolvers());
+    // Verify the timeout doesn't happen when disabled.
+    socket.setTimeout(0, () => {
+      hasTimeout = true;
+    });
+    await Bun.sleep(2000);
+    expect(hasTimeout).toBe(false);
+  }
+
+  {
+    // Verify the timeout doesn't happen after end().
+    let hasTimeout = false;
+    ({ promise, resolve } = Promise.withResolvers());
+    socket.setTimeout(1, () => {
+      hasTimeout = true;
+    });
+    otherSocket.end();
+    await Bun.sleep(2000);
+    expect(hasTimeout).toBe(false);
+  }
+}, 30_000);