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

[Feature] Voice reconnection and resuming #2873

Merged
merged 13 commits into from
Mar 14, 2024

Conversation

Lepterion
Copy link
Contributor

@Lepterion Lepterion commented Mar 8, 2024

Description

This PR adds missing reconnection and resume features to the AudioClient.

  1. Now the client reconnects when it's moved between channels (when a VOICE_STATE_UPDATE message saying the bot changed it's channel, following by VOICE_SERVER_UPDATE messages are received)
  2. When a connection was terminated and close code != 4001, 4002, 4003, 4004, 4005, 4006, 4009, 4012, 4016, the client'll try to resume lost connection. If it can't, the connection's stopped and the consumer needs to reconnect the client manually.

Changes

AudioClient:

  • added constant BlacklistedResumeCodes
  • added enum StopReason
  • added state fields: _stopReason and _resuming for disconnection control
  • don't forward ConnectionManager.Disconnected to AudioClient.Disconnected. This is done manually
  • method StopAsync() now calls internal overload StopAsync(StopReason) with StopReason.Normal
  • in OnConnectingAsync() depending on the _resuming value send either Identity or Resume message
  • added stop reason handling in the OnDisconnectingAsync(Exception) method
  • added methods FinishDisconnect and ClearHeartBeaters in order to make disconnect handling look better
  • cleaning up _keepaliveTimes
  • added method WaitForDisconnectAsync(TimeSpan)
  • handling OpCode 9 Resumed websocket message

ConnectionManager

  • swapped lines 175 and 176. This shouldn't affect anything but makes handling disconnection easier.

DiscordVoiceApiClient

  • added method SendResume

SocketGuild

  • in AddOrUpdateVoiceStateAsync stop the audio connection if moved or disconnected
  • in FinishConnectAudio if the connection is still active, wait until it closes (Doesn't block the gateway unless something's terribly wrong)

Other

  • added class Discord.API.Voice.ResumeParams

Related Issues

idk, this might be mentioned somewhere, but I couldn't find any related issues on GitHub.

@Lepterion
Copy link
Contributor Author

Lepterion commented Mar 8, 2024

builds failed 💀 ??????

@Misha-133
Copy link
Member

builds failed 💀 ??????

The old pipeline runs on .NET 6 and thus doesn't support .net7+ features
One of those being collection expressions

@Lepterion
Copy link
Contributor Author

Lepterion commented Mar 8, 2024

builds failed 💀 ??????

The old pipeline runs on .NET 6 and thus doesn't support .net7+ features One of those being collection expressions

How can I find out where the problem is?

@Misha-133
Copy link
Member

builds failed 💀 ??????

The old pipeline runs on .NET 6 and thus doesn't support .net7+ features One of those being collection expressions

How can I find out where is the problem?

image
image
image
image

@Lepterion
Copy link
Contributor Author

@Misha-133 resolved

@Misha-133 Misha-133 self-requested a review March 9, 2024 00:25
@Misha-133
Copy link
Member

@Lepterion
Thanks for updating this ancient part of DNet

I have a couple of questions tho.

  1. Could it be possible to keep the input stream in case of reconnects/channel switches?
  2. While testing this I got a
System.Threading.Tasks.TaskCanceledException: A task was canceled.
   at Discord.ConnectionManager.<>c__DisplayClass29_0.<<StartAsync>b__0>d.MoveNext() in H:\github\Discord.Net\src\Discord.Net.WebSocket\ConnectionManager.cs:line 79

and

System.Threading.Tasks.TaskCanceledException: A task was canceled.
   at Discord.WebSocket.SocketGuild.ConnectAudioAsync(UInt64 channelId, Boolean selfDeaf, Boolean selfMute, Boolean external, Boolean disconnect) in H:\github\Discord.Net\src\Discord.Net.WebSocket\Entities\Guilds\SocketGuild.cs:line 1802
   at Discord.WebSocket.SocketGuild.ConnectAudioAsync(UInt64 channelId, Boolean selfDeaf, Boolean selfMute, Boolean external, Boolean disconnect) in H:\github\Discord.Net\src\Discord.Net.WebSocket\Entities\Guilds\SocketGuild.cs:line 1807

repro steps:

  • connect the bot to a voice channel
  • move the bot to another channel through the UI
  • call ConnectAsync on the channel the bot was moved to

@Lepterion
Copy link
Contributor Author

@Misha-133

  1. I think no, because of session changes which cause change of sodium secrets.
  2. If it's a log message, probably, connection manager raises critical error in the task completion source. Technically, this can be resolved but idk if some of dnet's gateway features use this, so I'm afraid of modifying the connection manager.

@Lepterion
Copy link
Contributor Author

@Misha-133 task csncelled exception can be neverminded

@Misha-133
Copy link
Member

Misha-133 commented Mar 11, 2024

I think no, because of session changes which cause change of sodium secrets.

Yeah, but we could keep the stream alive on our end
So the playback can continue after resuming
Otherwise

don't forward ConnectionManager.Disconnected to AudioClient.Disconnected. This is done manually

would be a breaking change.

@Lepterion
Copy link
Contributor Author

would be a breaking change.

Is this very bad?

@Misha-133
Copy link
Member

Misha-133 commented Mar 11, 2024

We can't make breaking changes on non-major version bumps

https://github.com/discord-net/Discord.Net?tab=readme-ov-file#%EF%B8%8F-versioning-guarantees

@Lepterion
Copy link
Contributor Author

@Misha-133 I can create Finished event which will be invoked only when needed. Disconnected will be invoked on every connection termination.

@Misha-133
Copy link
Member

@Misha-133 I can create Finished event which will be invoked only when needed. Disconnected will be invoked on every connection termination.

That would still be a breaking change

@Lepterion
Copy link
Contributor Author

That would still be a breaking change

The reason for changing event's behaviour is that every time Disconnected is invoked, the client gets disposed. I can use audio client's internal state to resolve this issue instead of changing events.

@Lepterion
Copy link
Contributor Author

I can use audio client's internal state to resolve this issue instead of changing events.

@Misha-133 commited with this thing implemented

@Lepterion
Copy link
Contributor Author

@Misha-133 so what do you think?

@Misha-133
Copy link
Member

Yeah sorry for the delay, I'll take a look at it later

src/Discord.Net.WebSocket/Audio/AudioClient.cs Outdated Show resolved Hide resolved
src/Discord.Net.WebSocket/Audio/AudioClient.cs Outdated Show resolved Hide resolved
@Misha-133
Copy link
Member

Could you merge dev into this branch? so the workflow get's triggered

@Lepterion
Copy link
Contributor Author

Could you merge dev into this branch? so the workflow get's triggered

Dnet's dev into this?

@Lepterion
Copy link
Contributor Author

@Misha-133 done

@Misha-133 Misha-133 merged commit 09680c5 into discord-net:dev Mar 14, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants