Voice Gate: notify new unverified users in the verification channel#1272
Conversation
Signed-off-by: Daniel Brown <browndj3@gmail.com>
- Added RedisCache instance as a class attribute of the VoiceGate cog. - Added voice_gate channel as an attribute to use it later in the cog. - Added cache type hint. Signed-off-by: Daniel Brown <browndj3@gmail.com>
- Users who have never joined the voice channels before (and are currently unverified) will receive a ping in the #voice_verification channel - If user is unverified, the message id is stored in the cache along with the user id. - Added a message deletion to the voiceverify command, which removes the ping message if one exists. Also sets stored message ID to None so that it doesn't attempt to delete messages that aren't there - Set timed message deletion for 5 minutes.
- Corrected spelling on _async_init call - Changed the None value stored in the cache if the user is already verified to False, as RedisCache doesn't support None. - Changed RedisCache type hint to reflect change made in storage style - Suppress NotFound errors when the ping_message can't be retrieved. - Corrected lack of await on send call More fixes to come. Signed-off-by: Daniel Brown <browndj3@gmail.com>
- Added ping message constant to the top of the voice_gate.py file. - Corrected logic error in checking if a user is cached and in a voice channel. - Reduced default message deletion time to 1 minute from 5 minutes. - Adjusted on_message event to ignore the verification ping message. Signed-off-by: Daniel Brown <browndj3@gmail.com>
MrHemlock
added
a: backend
MrHemlock
requested review from
SebastiaanZ and
thomaspet
and removed request for
a team
Signed-off-by: Daniel Brown <browndj3@gmail.com>
…mlock/voice-gate-ping
Akarys42
left a comment
Akarys42
left a comment
There was a problem hiding this comment.
The PR looks good, there’s just one thing with how you handle deletion that could lead to trouble.
ghost
added
s: waiting for author
kwzrd
left a comment
kwzrd
left a comment
There was a problem hiding this comment.
Writing False into the cache feels a little hacky to me. How would you feel about writing a -1 or similar to signify that there's no message to be removed? It's not an ideal solution, ideally we'd want a set of pinged user IDs, but I think that's not currently possible with our wrapper, so "abusing" the keys sort of makes sense.
Akarys42
dismissed
their stale review
After some discussion in #dev-contrib, this solution turned to be working as fine as any other, there’s no actual reason to change it.
- Changed `VOICE_PING` constant to not contain the format brackets. - Added more detailed description of what the `redis_cache` will hold. - Changed message content verification to use the whole newly formatted `VOICE_PING` constant instead of a slice of it. - Added remaining parameters for the `on_voice_state_update` event for clarity. - Reorganized the logic of the `on_voice_state_update` for better clarity and better logging purposes. - Removed `_async_init` in favor of checking if the guild is ready inside the `on_voice_state_update` event. Verification channel is now loaded each time when needed, reducing risk of the object becoming stale or erroring out due to the not being ready before an event was triggered. Signed-off-by: Daniel Brown <browndj3@gmail.com>
ghost
added
needs 1 approval
and removed
s: waiting for author
- Added a None placeholder in the `__init__` for voice gate channel. - Changed deletion logic in on_voice_state_update to check if the message has already been deleted. - Changed deletion logic in voice_verify to only require one api call.
Since the cache offers a 'contains' coro, let's use it. If the member ID is already present in the cache, they were either already verified, or were already pung about not being verified.
The const was introduced for this purpose, but it was accidentally not being used.
This removes the need to fetch the Channel object. Add a trace log to help with testing.
Use a HTTP method so that we do not have to fetch the message object, the cache only gives us the ID.
|
We've agreed with @MrHemlock that I'd help address some of the feedback so that we can merge it soon. |
kwzrd
dismissed
their stale review
I'm going to dismiss my review since it's addressed now, but I want to play with the solution a little bit more before I approve & merge.
|
I'm spying a race issue here, when users might run into the pathological case when discord repeatedly spam joins/leaves the voice channel. This can result in over 6 joins/leave events per second, and while I'm not sure if the issue is fixed and do not have a way to reproduce it under my control (IIRC it relates to discord on android and at least another confounding factor), I think we should be careful and address that case. At the moment, the user-ping lockout in the cache is split over a send message and a wait-ready event, which makes it especially prone to getting re-entered in that situation, especially since there have been incidents where the bot takes 1-2 seconds to respond to things, depending on load and events happening elsewhere. We probably need a better lockout. Would it be reasonable to have a sentry "sending message" value to set into redis while we wait to send the message? |
thomaspet
left a comment
thomaspet
left a comment
There was a problem hiding this comment.
I am not apposed to the new changes, looks good still
|
@bast0006 You're definitely right that there's still plenty of opportunity for racing, both with concurrent voice events and the
Using Redis already comes with the problem that you can only interface with it asynchronously, so I don't think it's a bulletproof solution. A lock probably makes more sense, because you never have to I think I have an idea for how to solve it using a smart locking util that @MarkKoz wrote for another PR. It should allow us to prevent both the message & the voice state listener from runninng concurrently for the same user, using a decorator, which avoids having to add extra logic to the actual code. I'm planning to look at it soon. Edit: Using the deco lock would probably require a bit of a restructure because we don't want the voice listener to hold the lock hostage while it's waiting for the |
kwzrd
left a comment
kwzrd
left a comment
There was a problem hiding this comment.
I'm going to block this again until we make it async-safe.
ghost
added
the
s: waiting for author
The code for ping deletion was duplicated in two places. In this commit, we move it into a helper function, and apply a lock to make each transaction atomic. This means that if two coroutines try to call the function, the first has to finish before the second can begin. This avoids the following: Coro1: Message in cache? Yes. Coro1: Send delete request. Yield control (await). Coro2: Message in cache? Yes. Now Coro2 has to wait for Coro1 to finish. Therefore it will always find the `NO_MSG` signal, and not attempt the deletion. Co-authored-by: MarkKoz <KozlovMark@gmail.com> Co-authored-by: Sebastiaan Zeeff <sebastiaan.zeeff@gmail.com> Co-authored-by: Daniel Brown <browndj3@gmail.com>
Previously, the listener risked yielding control to a racing event at multiple points between checking whether the member was already notified, notifying them, and writing this information into the cache. As a result, in a pathological case, multiple racing coroutines could have passed the membership check and ping-spammed the user, before the first coro could have a chance to write the message ID into the cache. In this commit, we move this logic into an atomic helper, which will ensure that events are processed one-by-one, and subsequent events correctly abort. Co-authored-by: MarkKoz <KozlovMark@gmail.com> Co-authored-by: Sebastiaan Zeeff <sebastiaan.zeeff@gmail.com> Co-authored-by: Daniel Brown <browndj3@gmail.com>
ghost
removed
the
s: waiting for author
Previous changes reduced the spacing to 1 blank line, which is inconsistent with the prevailing style.
kwzrd
left a comment
kwzrd
left a comment
There was a problem hiding this comment.
Okay, I believe that this PR should no longer be prone to race conditions. The voice state listener performs two primary actions:
- Send the newcomer notification, if appropriate
- Delete the notification after the configured delay, if it hasn't been deleted yet
This first part suffered from a dangerous race condition, as pointed out by @bast0006. This has been addressed in 4b60c21 by making the whole procedure an atomic transaction. The second part was then prone to racing with the !voiceverify command, and is similarly made atomic by b4220a3.
I think this is now fine for production.
kwzrd
changed the title
thomaspet
left a comment
thomaspet
left a comment
There was a problem hiding this comment.
A side form ghostpings, which i suppose we can't do much about.. Still looks good
|
Everything is working as intended. Pushing her through. |
7 participants
This PR's purpose is to ping new users in the voice_verification channel so that they can figure out why they aren't able to talk. Currently, we have a lot of people confused about the new system. There are multiple people popping in and out of the voice channel thinking that there's something wrong with their connection or thinking they got specifically suppressed.
By pinging the user in the voice_verification channel (along with a little explanation on how to use it), this should reduce the number of folks asking.
The reason that this is only sending a ping in the channel and not a direct DM is that a fair number of people may have their DMs turned off. Another reason is that if they are joining a voice channel, they'll already be looking in that section of the channels, and a ping that's right in front of their eyes should get them looking in the right place.
Closes #1261