Rewrite voice connection internals #9525
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
imayhaveborkedit
force-pushed
the
voice_rewrite
branch
from
6b6cdf2
to
a471235
Compare
imayhaveborkedit
force-pushed
the
voice_rewrite
branch
from
c532928
to
ecbf885
Compare
Rapptz
reviewed
Aug 24, 2023
imayhaveborkedit
force-pushed
the
voice_rewrite
branch
from
a559c62
to
7dc0c4e
Compare
Rapptz
reviewed
Sep 10, 2023
Repository owner
deleted a comment
Sep 20, 2023
3 tasks
If this isn't the initial connection, the socket fills up with rtp/rtcp packets, meaning you can't just read one when you reconnect. This has terrible implications for voice recv.
I finally found this bug. Reconnecting sends a none speaking state, so we need to send speaking again to be heard.
also add ws property
Renamed VoiceClient._voice_state -> ._connection Renamed ConnectionStage -> ConnectionFlowState (tentatively) Renamed VoiceConnectionState._stage -> ._state Renamed VoiceConnectionState.stage -> .state Renamed parameters called stage -> state Renamed VoiceConnectionState._wait_for_stage -> ._wait_for_state Fix VoiceClient.wait_until_connected docstring
Rework ConnectionFlowState from bitfields to normal int enum and fix relevant comparisons. Remove obsolete exact param from _wait_for_state Remove IntEnum typing hack
Since we can only check absolute states now we need to be able to check for multiple in _wait_for_state
One step towards trying to avoid a very annoying problem with reading packets from the socket as a shared resource (voice recv)
We switch to force=True by default, and now have a cleanup param to selectively run voice client cleanup.
The voice socket is no longer directly read from by the voice websocket to get the ip discovery packet. Now there is a socket reader thread object that handles this by using registered callbacks. This way there is no longer any for the socket to be improperly shared between any potential readers. In the base library this isn't important since the only thing the socket is read for is the ip discovery packet, but for *extensions implementing voice recv* its necessary. The thread pauses when there are no listeners attached as not to waste cpu time.
Clean up ip discovery future handling Rename _self_paused to _idle_paused Misc
Fixing checks and timeouts mostly. The change to move_to is mostly a band-aid fix. It doesn't solve the underlying problem but it removes that vector for causing it.
By moving connect() to a task we can cancel it and restart, making it easier to handle overlapping reconnects. It seems to work from my testing but i'm not entirely sure its sound, specifically the whole 'stopping the ws runner task' part. If anything its racy but that remains to be seen.
There are a bunch of TODOs that need input or a decision on
Remove ip caching, didn't really serve a purpose Add timeout param to move_to() Change BlockingIOError to OSError in send_audio_packet() Change task types to Optional instead of MISSING Properly transfer connect params and voice state to connect() calls Remove some uncertain code path handling TODOs Fix reconnecting in the ws runner task
Raise errors on connection problems Add default timeout to wait_until_connected Undocument wait_until_connected (its sync and only used internally once) Change some timeouts to 30s Floatify floats
imayhaveborkedit
force-pushed
the
voice_rewrite
branch
from
8530f6d
to
92e2bbb
Compare
Rapptz
reviewed
Sep 22, 2023
|
Alright I think that's finally everything. I'm not entirely satisfied with how the overall design feels but it's more stable so good enough for now. Maybe if it needs to be worked on again i'll rewrite it completely but maybe by then it won't be my job. Edit: Except for this stuff. NOW we're done. |
Good thing I remembered to do this or fixing this in my voice recv code would have been a little annoying.
Rapptz
reviewed
Sep 28, 2023
|
🎉 |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
I have taken the liberty of rewriting the voice connection code to deal with various issues caused by the current one. Essentially, I have moved all of the connection related code out of
voice_client.pyinto the newvoice_state.py. In the process, while the code remains largely the same overall, I have converted it to a state machine instead of relying entirely on Events for state. This should be all internal changes not breaking anything outwards facing, but I'm not 100% sure I didn't accidentally break something.This code is in a done enough state to be reviewed but I still have a couple things left to finalize.
Fixes #2284
Fixes #4091
Fixes #8483
Fixes #9039
Fixes #9137
Fixes #9364
Fixes #9575
There may be other issues fixed by this incidentally. I will bump several issues after the merge.
Checklist