Skip to content

Commit

Permalink
fix: fix socket.disconnect().connect() usage
Browse files Browse the repository at this point in the history
Previously, calling `socket.disconnect().connect()` could, if the
connection was upgraded to WebSocket, result in "disconnect" being
emitted twice, and an engine being leaked.

Here's what happened:

> socket.disconnect()

- calls `socket.destroy()` so the socket doesn't listen to the manager events anymore
- then calls `manager._close()` which closes the underlying engine but not the manager itself (it waits for the "close" event of the engine)

> socket.connect()

- calls `socket.subEvents()` so the socket does listen to the manager events
- calls `manager.open()` which creates a new engine

And then the first engine emits a "close" event, which is forwarded to
the socket, hence the second "disconnect" event.

Related: #1014
  • Loading branch information
darrachequesne committed Nov 18, 2021
1 parent 53d8fca commit 99c2cb8
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 8 deletions.
10 changes: 2 additions & 8 deletions lib/manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -520,13 +520,7 @@ export class Manager<
debug("disconnect");
this.skipReconnect = true;
this._reconnecting = false;
if ("opening" === this._readyState) {
// `onclose` will not fire because
// an open event never happened
this.cleanup();
}
this.backoff.reset();
this._readyState = "closed";
this.onclose("forced close");
if (this.engine) this.engine.close();
}

Expand All @@ -545,7 +539,7 @@ export class Manager<
* @private
*/
private onclose(reason: string): void {
debug("onclose");
debug("closed due to %s", reason);

this.cleanup();
this.backoff.reset();
Expand Down
19 changes: 19 additions & 0 deletions test/socket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,25 @@ describe("socket", function () {
}, 100);
});

it("should properly disconnect then reconnect", (done) => {
const socket = io("/", { forceNew: true, transports: ["websocket"] });

let count = 0;

socket.once("connect", () => {
socket.disconnect().connect();
});

socket.on("disconnect", () => {
count++;
});

setTimeout(() => {
expect(count).to.eql(1);
success(done, socket);
}, 200);
});

it("should throw on reserved event", () => {
const socket = io("/no", { forceNew: true });

Expand Down

0 comments on commit 99c2cb8

Please sign in to comment.