From 86c950cd594dabfea37a21317c09e9be27598ed5 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 24 Jul 2024 03:06:19 -0700 Subject: [PATCH] Fix net.Socket timeout behavior --- src/bun.js/api/bun/socket.zig | 6 ++-- src/js/node/net.ts | 49 ++++++++++++++++++++++++------- test/js/node/net/node-net.test.ts | 49 +++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 12 deletions(-) diff --git a/src/bun.js/api/bun/socket.zig b/src/bun.js/api/bun/socket.zig index d6642fcf17..78b969370f 100644 --- a/src/bun.js/api/bun/socket.zig +++ b/src/bun.js/api/bun/socket.zig @@ -1189,7 +1189,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, @@ -1750,12 +1749,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(); } diff --git a/src/js/node/net.ts b/src/js/node/net.ts index ee6e63a115..a6498f34ea 100644 --- a/src/js/node/net.ts +++ b/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; } diff --git a/test/js/node/net/node-net.test.ts b/test/js/node/net/node-net.test.ts index 6fa7758875..6acd6a2b0b 100644 --- a/test/js/node/net/node-net.test.ts +++ b/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);