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.

Sign up for GitHub

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

Migrate on disconnect #31

Merged
merged 8 commits into from
Dec 3, 2023
Merged

Migrate on disconnect #31

merged 8 commits into from
Dec 3, 2023

Conversation

freyacodes
Copy link
Contributor

Contains all changes from #30 and most of #28, on which this PR is built upon.

Adds a new option that is enabled by default:

Whether to try to migrate links from a disconnected node onto a new one. This option has no effect if [autoReconnect] is false. If the node is trying to resume, the migration will only take place after the node gives up on resuming as per [retry].

@freyacodes freyacodes force-pushed the migrate-on-disconnect branch 2 times, most recently from f7ba809 to 4252855 Compare October 16, 2023 08:56
@DRSchlaubi
Copy link
Member

Pls rebase again

@freyacodes
Copy link
Contributor Author

freyacodes commented Oct 16, 2023 •
edited
Loading

I'm currently investigating a bug that seems to be related to this PR, where the WebSocket is blocked from reconnecting

(Will rebase soon)

@freyacodes freyacodes force-pushed the migrate-on-disconnect branch from 4252855 to ee12b48 Compare October 16, 2023 13:40
@freyacodes
Copy link
Contributor Author

I believe that something caused the joinAll() call to suspend indefinitely when a node which wasn't actually connected was assigned as the new node of the link (in some cases due to a race condition). I don't know what would cause the indefinite suspension, but I believe I fixed the bad node assignment.

@freyacodes
Copy link
Contributor Author

Currently, this PR as well as Lavalink.kt more generally assumes that the state of a link is CONNECTED after voice data is sent to Lavalink. Should it not remain in CONNECTING until Lavalink notifies the client that it is connected to the voice gateway?

@freyacodes
Copy link
Contributor Author

From the kdoc:

We have dispatched the voice server info to the server, and it should (soon) be connected.

I do realise that CONNECTED does not strictly mean "connected", but I think this could be improved as Lavalink does communicate when it is and isn't connected to the voice gateway. It could however be seen as a breaking change.

@DRSchlaubi
Copy link
Member

I think I copied that from the og client, but yeah sure, sounds like a reasonable improvment

@freyacodes
Change voice connection handling
32c40cc 
- Rely on player state for CONNECTED state
- Do not destroy player on disconnect
- Improved debug logging in some places
@freyacodes
Fix not emitting WebSocketClosedEvent
44d1e99
@freyacodes
Merge branch 'main' into migrate-on-disconnect
dfab077
@DRSchlaubi DRSchlaubi merged commit 7ee242e into kordlib:main Dec 3, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@freyacodes @DRSchlaubi