Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(WebSocketShard): wsCloseTimeout in wrong if #8981

Closed
wants to merge 3 commits into from

Conversation

Qjuh
Copy link
Contributor

@Qjuh Qjuh commented Dec 26, 2022

Please describe the changes this PR makes and why it should be merged:
Fixes issues introduced by #8956 because it added the wsCloseTimeout even when the Websocket was already Closed before. This lead to the timeout trying to close the new connection after it successfully reconnected.
Also added a clearing of the wsCloseTimeout before making a new connection to prevent any other obscure occurences like this.

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

@vercel
Copy link

vercel bot commented Dec 26, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated
discord-js ⬜️ Ignored (Inspect) Dec 26, 2022 at 3:45PM (UTC)
discord-js-guide ⬜️ Ignored (Inspect) Dec 26, 2022 at 3:45PM (UTC)

Comment on lines +823 to +828
this.closeEmitted = false;
this.debug(
`[WebSocket] Adding a WebSocket close timeout to ensure a correct WS reconnect.
Timeout: ${this.manager.client.options.closeTimeout}ms`,
);
this.setWsCloseTimeout(this.manager.client.options.closeTimeout);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are very similar to the
file: https://github.com/legendhimself/discord.js/blob/01797d3b0cd316a97853f34a971034c1904b4efb/packages/discord.js/src/client/websocket/WebSocketShard.js#L824
pr: #7626

this.connection.close(closeCode);

Once this ^ is invoked the connection is either CLOSING or CLOSED state.

Copy link
Contributor Author

@Qjuh Qjuh Dec 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only CLOSING state, never CLOSED (because it was OPEN before the .close() call), so yes it is similar, but not exactly the same. Especially because there are other reasons why the Websocket could be CLOSED that shouldn't need a wsCloseTimeout because they already closed the Websocket.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLOSED check was added for very large scaled bots (Just to cover every case). And after close() is invoked the ws waits for the close frame and then marks the connection CLOSED. Receiving that close frame differs (might be instant and can happen when the execution is in the current scope) hence that's why I had the check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Websocket doesn’t wait for the close frame though… it’s an async callback. So that‘ll never happen while in this scope, status will always be CLOSING here…

@DraftProducts
Copy link
Contributor

DraftProducts commented Jan 2, 2023

It can be closed, it's superseded by #8989

@Jiralite Jiralite closed this Jan 2, 2023
@Qjuh Qjuh deleted the fix-ws-close-timeout branch January 7, 2023 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants