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 bot voice state connection issues #1533

Merged

Conversation

tequa
Copy link
Contributor

@tequa tequa commented Aug 16, 2023

Pull Request Type

  • Feature addition
  • Bugfix
  • Documentation update
  • Code refactor
  • Tests improvement
  • CI/CD pipeline enhancement
  • Other: [Replace with a description]

Description

I encountered a bug where connecting and disconnecting the bot itself across different guilds caused weird state issues to happen. Like if you disconnect the bot from a voice channel in one guild, the bot in the other guild wouldn't disconnect properly. See linked-issue for full context.

The issue with this in voice_events.py. The problem is that that it's grabbing cached before data for the user ID, which is fine if you're doing voice updates for actual non-bot users, but if this event is happening on a bot, it's gonna pull the "user" data for that bot, which will be global across guilds. I updated the code so that the bot updates ignore the before data and don't dispatch a voice state update, since this voice state update would be global across all guilds and not useful in any way. This fixed the bug I was encountering.

Changes

Related Issues

Issue 1531

Test Scenarios

Python Compatibility

  • I've ensured my code works on Python 3.10.x
  • I've ensured my code works on Python 3.11.x

Checklist

  • I've run the pre-commit code linter over all edited files
  • I've tested my changes on supported Python versions
  • I've added tests for my code, if applicable
  • I've updated / added documentation, where applicable

Copy link
Member

@AstreaTSS AstreaTSS left a comment

Choose a reason for hiding this comment

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

As I mentioned, it's unacceptable for bots not to be dispatched - that would both be arguably breaking and removing functionality from interactions.py. I believe this solution needs to be redone in some manner.

Copy link
Member

@AstreaTSS AstreaTSS left a comment

Choose a reason for hiding this comment

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

Looks mostly good after that fix - just a few things.

interactions/api/events/processors/voice_events.py Outdated Show resolved Hide resolved
interactions/api/events/processors/voice_events.py Outdated Show resolved Hide resolved
AstreaTSS
AstreaTSS previously approved these changes Aug 16, 2023
@AstreaTSS AstreaTSS dismissed their stale review August 16, 2023 21:18

Invalidated due to recent comments.

Copy link
Member

@AstreaTSS AstreaTSS left a comment

Choose a reason for hiding this comment

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

Looks good, though because this PR went through a lot, I'm going to wait for someone else's approval instead of doing it myself.

@silasary
Copy link
Member

So, I hate to do this, but... This still doesn't correctly handle a non-bot user being in two voice channels at once 😛

@silasary
Copy link
Member

It definitely fixes a bug short-term, but it doesn't fix the root bug.

@AstreaTSS
Copy link
Member

So, I hate to do this, but... This still doesn't correctly handle a non-bot user being in two voice channels at once 😛

Well, yes, but there's no solution to that without a breaking change.

@tequa
Copy link
Contributor Author

tequa commented Aug 20, 2023

So, I hate to do this, but... This still doesn't correctly handle a non-bot user being in two voice channels at once 😛

The code around non-bot users shouldn't have changed at all with this MR -- the before, after, and dispatch events should remain the same.

Also, I didn't think a non-bot user could be in 2 voice channels at once? When I join a new voice channel in the same guild or even a new guild, it leaves the old voice channel.

@AstreaTSS
Copy link
Member

Indeed, a non-bot user can't be in two voice channels at once... but a bot user can be in multiple. With the way interactions.py is designed, it only accounts for this fact for the running bot, not any other bot.

Again, this is unfixable without changing the cache of interactions.py, which would be breaking.

@silasary
Copy link
Member

silasary commented Aug 20, 2023

You can be, you just need two instances of the client open (Either Desktop app and Browser/PTB/Canary, or two computers, but apparently not PC + Mobile?)

@AstreaTSS
Copy link
Member

AstreaTSS commented Aug 20, 2023

You can be, you just need two instances of the client open (Either Desktop app and Browser/PTB/Canary, or two computers, or PC + Mobile... etc)

Um, Discord disconnects you if you do that.

EDIT: Turns out it sometimes doesn't. Discord moment.

Copy link
Member

@silasary silasary left a comment

Choose a reason for hiding this comment

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

This is the best we'll get before 6.0, so I'm happy enough

Copy link
Contributor

@LordOfPolls LordOfPolls left a comment

Choose a reason for hiding this comment

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

Happy with this, but the voice state cache needs an overhaul due to oversights in its design

@silasary silasary merged commit 69d187f into interactions-py:unstable Aug 26, 2023
1 check 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.

4 participants