diff --git a/packages/socket.io-client/lib/socket.ts b/packages/socket.io-client/lib/socket.ts index a1713a187b..bf26c373c7 100644 --- a/packages/socket.io-client/lib/socket.ts +++ b/packages/socket.io-client/lib/socket.ts @@ -246,27 +246,37 @@ export class Socket< * * The `withError` attribute is used to differentiate handlers that accept an error as first argument: * - * - `socket.emit("test", (err, value) => { ... })` with `ackTimeout` option * - `socket.timeout(5000).emit("test", (err, value) => { ... })` * - `const value = await socket.emitWithAck("test")` * * From those that don't: * * - `socket.emit("test", (value) => { ... });` + * - `socket.emit("test", (value) => { ... })` with `ackTimeout` option (success case) * - * In the first case, the handlers will be called with an error when: + * The `hasTimeout` attribute indicates whether a timeout was configured (either via `.timeout()` or `ackTimeout` option). + * This is used for timeout handling, but NOT for disconnection handling. + * + * In the first case (withError=true), the handlers will be called with an error when: * * - the timeout is reached * - the socket gets disconnected * - * In the second case, the handlers will be simply discarded upon disconnection, since the client will never receive + * In the second case (withError=false but hasTimeout=true), the handlers will NOT be called with an error: + * + * - on timeout: callback is silently discarded (callback signature is (value) => {} and doesn't accept errors) + * - on disconnection: callback is silently discarded (callback signature is (value) => {} and doesn't accept errors) + * + * Successful responses will NOT prepend null (only when withError=true). + * + * In the third case (no timeout), the handlers will be simply discarded upon disconnection, since the client will never receive * an acknowledgement from the server. * * @private */ private acks: Record< string, - ((...args: any[]) => void) & { withError?: boolean } + ((...args: any[]) => void) & { withError?: boolean; hasTimeout?: boolean } > = {}; private flags: Flags = {}; private subs?: Array; @@ -463,30 +473,66 @@ export class Socket< */ private _registerAckCallback(id: number, ack: (...args: any[]) => void) { const timeout = this.flags.timeout ?? this._opts.ackTimeout; - if (timeout === undefined) { + const hasExplicitTimeout = this.flags.timeout !== undefined; + const isFromQueue = this.flags.fromQueue === true; + + // If packet is from queue, we need to wrap the callback even without timeout + // to ensure onack prepends null (via withError flag) so the queue callback + // receives (null, ...responseArgs) format + if (timeout === undefined && !isFromQueue) { this.acks[id] = ack; return; } - // @ts-ignore - const timer = this.io.setTimeoutFn(() => { - delete this.acks[id]; - for (let i = 0; i < this.sendBuffer.length; i++) { - if (this.sendBuffer[i].id === id) { - debug("removing packet with ack id %d from the buffer", id); - this.sendBuffer.splice(i, 1); + // Create a wrapper function that will handle the callback + // Determine if withError will be set (needed for timeout handler) + const willHaveWithError = hasExplicitTimeout || isFromQueue; + let timer; + if (timeout !== undefined) { + // @ts-ignore + timer = this.io.setTimeoutFn(() => { + delete this.acks[id]; + for (let i = 0; i < this.sendBuffer.length; i++) { + if (this.sendBuffer[i].id === id) { + debug("removing packet with ack id %d from the buffer", id); + this.sendBuffer.splice(i, 1); + } } - } - debug("event with ack id %d has timed out after %d ms", id, timeout); - ack.call(this, new Error("operation has timed out")); - }, timeout); + debug("event with ack id %d has timed out after %d ms", id, timeout); + // Only call with error if callback expects an error parameter (withError=true) + // When only ackTimeout is set (without explicit .timeout() and not from queue), + // the callback signature is (value) => {} and should not receive an error on timeout + if (willHaveWithError) { + ack.call(this, new Error("operation has timed out")); + } + // Otherwise, silently discard the callback (similar to disconnection behavior) + }, timeout); + } const fn = (...args: any[]) => { - // @ts-ignore - this.io.clearTimeoutFn(timer); + if (timer !== undefined) { + // @ts-ignore + this.io.clearTimeoutFn(timer); + } + // onack will prepend null when withError is true, so args already contains (null, ...responseArgs) + // for queued packets or explicit timeout cases ack.apply(this, args); }; - fn.withError = true; + + // Set hasTimeout flag when any timeout is configured (explicit or ackTimeout) + // This is used to determine whether to call with error upon disconnection + if (timeout !== undefined) { + fn.hasTimeout = true; + } + + // Set withError flag when: + // 1. Explicitly using .timeout() - user expects (err, response) signature + // 2. Packet is from queue - queue's internal callback expects (err, response) signature + // onack will prepend null when this flag is set + // Otherwise, when only ackTimeout is set, don't set withError to preserve (response) signature + if (hasExplicitTimeout || isFromQueue) { + fn.withError = true; + } this.acks[id] = fn; } @@ -682,13 +728,19 @@ export class Socket< (packet) => String(packet.id) === id, ); if (!isBuffered) { - // note: handlers that do not accept an error as first argument are ignored here const ack = this.acks[id]; delete this.acks[id]; + // Only call with error if the callback expects an error parameter (withError=true) + // When only ackTimeout is set (without explicit .timeout()), the callback signature + // is (value) => {} and should not receive an error on disconnection if (ack.withError) { ack.call(this, new Error("socket has been disconnected")); } + // note: handlers without withError flag are ignored here, since the client will never receive + // an acknowledgement from the server. This includes: + // - handlers without timeout + // - handlers with only ackTimeout (but not explicit .timeout()) } }); } diff --git a/packages/socket.io-client/test/node.ts b/packages/socket.io-client/test/node.ts index b1b8112fa3..d2639c8e32 100644 --- a/packages/socket.io-client/test/node.ts +++ b/packages/socket.io-client/test/node.ts @@ -28,12 +28,19 @@ describe("autoUnref option", function () { }); it("should not stop with autoUnref set to false", (done) => { + let doneCalled = false; const process = exec(fixture("no-unref.ts"), () => { - done(new Error("should not happen")); + if (!doneCalled) { + doneCalled = true; + done(new Error("should not happen")); + } }); setTimeout(() => { process.kill(); - done(); + if (!doneCalled) { + doneCalled = true; + done(); + } }, 100); }); }); diff --git a/packages/socket.io-client/test/retry.ts b/packages/socket.io-client/test/retry.ts index 9ceca208b1..7f6f6058c3 100644 --- a/packages/socket.io-client/test/retry.ts +++ b/packages/socket.io-client/test/retry.ts @@ -146,4 +146,31 @@ describe("retry", () => { }, 100); }); }); + + it("should handle queued packets without timeout correctly", () => { + return wrap((done) => { + const socket = io(BASE_URL, { + forceNew: true, + retries: 1, + // No ackTimeout or explicit timeout + }); + + let responseReceived = false; + + socket.emit("echo", "test", (err, val) => { + // Queue callback expects (err, ...responseArgs) signature + expect(err).to.be(null); + expect(val).to.eql("test"); + responseReceived = true; + success(done, socket); + }); + + // Wait a bit to ensure the response is processed + setTimeout(() => { + if (!responseReceived) { + done(new Error("Response not received")); + } + }, 500); + }); + }); }); diff --git a/packages/socket.io-client/test/socket.ts b/packages/socket.io-client/test/socket.ts index 9374453850..b28e097d01 100644 --- a/packages/socket.io-client/test/socket.ts +++ b/packages/socket.io-client/test/socket.ts @@ -657,10 +657,19 @@ describe("socket", () => { ackTimeout: 50, }); - socket.emit("unknown", (err) => { - expect(err).to.be.an(Error); - success(done, socket); + // When only ackTimeout is set (without explicit .timeout()), the callback + // signature is (value) => {} and should not receive an error on timeout + // The timeout will silently discard the callback + socket.emit("unknown", (_value) => { + done(new Error("should not happen")); }); + + // Wait for timeout to occur + setTimeout(() => { + // @ts-ignore property 'acks' is private + expect(Object.keys(socket.acks).length).to.eql(0); + success(done, socket); + }, 100); }); }); @@ -707,7 +716,7 @@ describe("socket", () => { }); }); - it("should ack with an error upon disconnection (callback & ackTimeout)", () => { + it("should not ack upon disconnection (callback & ackTimeout)", () => { return wrap((done) => { const socket = io(BASE_URL, { forceNew: true, @@ -715,13 +724,18 @@ describe("socket", () => { }); socket.on("connect", () => { - socket.emit("echo", "a", (err) => { - expect(err).to.be.an(Error); - - success(done, socket); + // When only ackTimeout is set (without explicit .timeout()), the callback + // signature is (value) => {} and should not receive an error on disconnection + socket.emit("echo", "a", (_value) => { + done(new Error("should not happen")); }); socket.disconnect(); + + // @ts-ignore property 'acks' is private + expect(Object.keys(socket.acks).length).to.eql(0); + + setTimeout(() => success(done, socket), 100); }); }); }); @@ -788,6 +802,52 @@ describe("socket", () => { }); }); + describe("acknowledgment callback signature", () => { + it("should have consistent callback signature when emitting before and after connection with ackTimeout", () => { + return wrap((done) => { + const socket = io(BASE_URL, { + forceNew: true, + ackTimeout: 5000, // Set ackTimeout but don't use .timeout() + }); + + let beforeConnectArgs: any[] | null = null; + let afterConnectArgs: any[] | null = null; + + // Emit before connection - callback should receive 1 argument (response) + socket.emit("echo", "test-before", function () { + beforeConnectArgs = Array.from(arguments); + // Verify callback receives only 1 argument (the response) + expect(beforeConnectArgs.length).to.eql(1); + expect(beforeConnectArgs[0]).to.eql("test-before"); + + // Check that after connection callback was also called + if (afterConnectArgs !== null) { + expect(afterConnectArgs.length).to.eql(1); + expect(afterConnectArgs[0]).to.eql("test-after"); + success(done, socket); + } + }); + + socket.on("connect", () => { + // Emit after connection - callback should receive 1 argument (response) + socket.emit("echo", "test-after", function () { + afterConnectArgs = Array.from(arguments); + // Verify callback receives only 1 argument (the response) + expect(afterConnectArgs.length).to.eql(1); + expect(afterConnectArgs[0]).to.eql("test-after"); + + // Check that before connection callback was also called + if (beforeConnectArgs !== null) { + expect(beforeConnectArgs.length).to.eql(1); + expect(beforeConnectArgs[0]).to.eql("test-before"); + success(done, socket); + } + }); + }); + }); + }); + }); + describe("throttled timer", () => { it("should buffer the event and send it upon reconnection", () => { return wrap((done) => {