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

Jump to bottom

fix(websocketshard): deal with zombie connection caused by 4009 #7581

Merged
merged 51 commits into from Jun 5, 2022
Merged

fix(websocketshard): deal with zombie connection caused by 4009 #7581

merged 51 commits into from Jun 5, 2022

Conversation

legendhimself
Copy link
Contributor

Please describe the changes this PR makes and why it should be merged:
Fixes: #7450
This pr deals with the zombie connection and reconnections of the WebSocket, The issue was readyState being stuck at CLOSING indefinitely when there was a session timeout [4009].

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
  • This PR changes the library's interface (methods or parameters added)

@legendhimself legendhimself mentioned this pull request Mar 1, 2022
Closed
1 task
almeidx
almeidx reviewed Mar 1, 2022
View reviewed changes
Copy link
Member

@almeidx almeidx left a comment

Choose a reason for hiding this comment

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

In order to not spam the pull request, please use the enum instead of a magic number in this.connection?.readyState ?? 3 (there's at least 10 references of this in this pull request)

almeidx
almeidx requested changes Mar 1, 2022
View reviewed changes
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
@almeidx
Copy link
Member

almeidx commented Mar 1, 2022

Is this issue not present on the main branch?

@imranbarbhuiya
Copy link
Contributor

Is this issue not present on the main branch?

it is. But we were testing in v13 branch so created pr for v13 first. He'll create another one for main.

legendhimself and others added 2 commits March 1, 2022 17:13
vvito7
vvito7 reviewed Mar 1, 2022
View reviewed changes
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
@legendhimself @vvito7
Apply suggestions from code review
ac2beb3 
Co-authored-by: Vitor <milagre.vitor@gmail.com>
@vvito7
Copy link
Contributor

vvito7 commented Mar 1, 2022

image
Is it necessary to set the status twice?

@legendhimself
Copy link
Contributor Author

image Is it necessary to set the status twice?

its unnecessary.

@legendhimself
Copy link
Contributor Author

legendhimself commented Mar 1, 2022 •
edited

Before It used to just go offline ie the child process was still alive, but the WebSocket was at the closing state and unresponsive ie zombie connection. You can refer to the issue mentioned in this prs description for more details

Now after pushing my fixes in production on my bot the reconnects are smooth for all codes, after 4009 it reconnects smoothly.

It's weird that only the 5th shard out of 12 experiences this 4009 session timeout issue, the rest of the shards are fine with weeks of uptime and now my fixes work for that shard as well

Uptime:
image

Logs:
image

@kyranet kyranet added this to the discord.js v13.7 milestone Mar 1, 2022
SpaceEEC
SpaceEEC previously requested changes Mar 6, 2022
View reviewed changes
Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

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

🙏 debug strings

Just these small things:

src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
@legendhimself @SpaceEEC
Apply suggestions from code review
e80a867 
Co-authored-by: SpaceEEC <spaceeec@yahoo.com>
vladfrangu
vladfrangu requested changes May 14, 2022
View reviewed changes
src/client/websocket/WebSocketManager.js Outdated Show resolved Hide resolved
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
src/util/Options.js Show resolved Hide resolved
Jiralite
Jiralite requested changes May 16, 2022
View reviewed changes
src/util/Options.js Outdated Show resolved Hide resolved
vladfrangu
vladfrangu approved these changes May 16, 2022
View reviewed changes
Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

All the [WebSocket] log message prefixes are not really useful (this.debug already appends a prefix), but this looks fine

@imranbarbhuiya imranbarbhuiya mentioned this pull request May 24, 2022
Closed
@legendhimself legendhimself marked this pull request as draft May 26, 2022 01:36
@legendhimself
fix: deal with an edge case
88fb5f6 
Invoke destroy when connection object exists in onClose method for a cleanup and correct reconnect.
@legendhimself legendhimself marked this pull request as ready for review May 29, 2022 07:07
@legendhimself
Copy link
Contributor Author

legendhimself commented May 29, 2022 •
edited

After a few days of testing this code. I think I covered/fixed most of the edge cases where WS fails to reconnect or used to go into reconnecting loop.
@kyranet Please review

@kyranet kyranet linked an issue May 29, 2022 that may be closed by this pull request
Unhandled 'error' event: "WebSocket was closed before the connection was established" when client was connected and network goes down #7964
Closed
kyranet
kyranet approved these changes May 29, 2022
View reviewed changes
Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

I added #7964 to the list of issues this PR fixes.

I also don't know if we'll see another v13 version but... I'll approve just in case.

@BenPatersonxx

This comment was marked as spam.

Sign in to view
@iCrawl iCrawl merged commit 114bcc0 into discordjs:v13 Jun 5, 2022
@CodemonkeyOSS CodemonkeyOSS mentioned this pull request Jun 11, 2022
Merged
@legendhimself legendhimself mentioned this pull request Jul 11, 2022
Merged
14 tasks
@RedGuy12 RedGuy12 mentioned this pull request Oct 4, 2022
Closed
@RedGuy12 RedGuy12 mentioned this pull request Oct 30, 2022
Merged
@legendhimself legendhimself mentioned this pull request Nov 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@almeidx almeidx almeidx requested changes

@vvito7 vvito7 vvito7 left review comments

@Jiralite Jiralite Jiralite requested changes

@imranbarbhuiya imranbarbhuiya imranbarbhuiya requested changes

@vladfrangu vladfrangu vladfrangu approved these changes

@iCrawl iCrawl iCrawl approved these changes

@kyranet kyranet kyranet approved these changes

@SpaceEEC SpaceEEC SpaceEEC approved these changes

@MattTheCuber MattTheCuber MattTheCuber approved these changes

Assignees
No one assigned
Labels
gateway packages:discord.js semver:patch
Projects
discord.js
Archived in project
Milestone
discord.js v13.7