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

feat(locks): member locks #1172

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Conversation

JPBM135
Copy link
Contributor

@JPBM135 JPBM135 commented Jan 11, 2023

Why?

Duplicated actions have been a rare but occurrent problem, this PR uses Redis to create a one-time member lock to prevent more than one mod to action a member at the same time

Fixes #1149

Const

  • MEMBER_LOCK_EXPIRE_SECONDS using 20 seconds is the value I found to be between the 15 seconds action timer and a too-big value

TODO:

  • Use a try...finally style
  • Find a way to track expired locks

Note
While this logic has been implemented in anti-raid-nuke, I'm still not sure about it (Removed)

@vercel
Copy link

vercel bot commented Jan 11, 2023

@JPBM135 is attempting to deploy a commit to the discordjs Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Jan 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
yuudachi-report ⬜️ Ignored (Inspect) Jan 13, 2023 at 4:45PM (UTC)

Copy link
Contributor

@appellation appellation left a comment

Choose a reason for hiding this comment

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

This looks decent, but a couple points to consider:

  1. Can we move the locking into createCase? That's really where it matters, and might cut down on code duplication.
  2. I'd really like to see locking handled with a try...finally block to ensure the lock is properly disposed once the resource is no longer needed. Unfortunately JS doesn't have a good way to do this, so I'd recommend either just always using a try...finally block or passing a callback to the locking function with the code that requires the lock; callback is probably the cleanest and most explicit about the lock boundaries.

apps/yuudachi/src/functions/anti-raid/blastOff.ts Outdated Show resolved Hide resolved
apps/yuudachi/src/functions/locks/locks.ts Outdated Show resolved Hide resolved
@JPBM135
Copy link
Contributor Author

JPBM135 commented Jan 11, 2023

This looks decent, but a couple points to consider:

  1. Can we move the locking into createCase? That's really where it matters, and might cut down on code duplication.
  2. I'd really like to see locking handled with a try...finally block to ensure the lock is properly disposed once the resource is no longer needed. Unfortunately JS doesn't have a good way to do this, so I'd recommend either just always using a try...finally block or passing a callback to the locking function with the code that requires the lock; callback is probably the cleanest and most explicit about the lock boundaries.

1- The thing about moving into createCase is, it does not solve possible case duplication, since a mod can be in the confirmation screen and another one triggering the action, since all the tests and validations are made prior to confirmation there is no current way to check this

2- I couldn't find a good way to do this either, but I will deff give another look into it

@JPBM135 JPBM135 marked this pull request as draft January 11, 2023 06:59
@JPBM135
Copy link
Contributor Author

JPBM135 commented Jan 11, 2023

2. I'd really like to see locking handled with a try...finally block to ensure the lock is properly disposed once the resource is no longer needed. Unfortunately JS doesn't have a good way to do this, so I'd recommend either just always using a try...finally block or passing a callback to the locking function with the code that requires the lock; callback is probably the cleanest and most explicit about the lock boundaries.

Found a solution for this, implemented the member lock in the command handler, only using extends when needed

@appellation
Copy link
Contributor

Found a solution for this, implemented the member lock in the command handler, only using extends when needed

Neat that's pretty clean.

@JPBM135 JPBM135 marked this pull request as ready for review January 13, 2023 16:46
@almostSouji almostSouji requested a review from iCrawl June 9, 2023 09:58
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.

bug(cases): duplicated cases if created in succession
2 participants