-
Notifications
You must be signed in to change notification settings - Fork 139
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: add a lock around interaction responses to fix race conditions in case multiple responses are made at once #496
base: master
Are you sure you want to change the base?
Conversation
@shiftinv Hi, would you please consider reviewing this pr? |
The changes themselves look good, but in a more general sense this seems like something that should be handled by users, not by the lib, imo. lock = asyncio.Lock()
async def maybe_respond_after_two_seconds(num):
await asyncio.sleep(2)
async with lock:
if inter.response.is_done():
return
await inter.response.send_message(f"hi {num}") |
The issue I see is that A different solution would be try/except and use the followup if we catch an already responded error in |
Good point, it shouldn't really be doing that as it stands right now, though sending two responses at the same time isn't something we've explicitly supported so far.
Either that, or we could improve the documentation regarding simultaneous responses. Reading through the code again, I think I got the wrong impression at first - the purpose of this change is essentially to raise the correct exception type, instead of only adding a lock around responses? Perhaps that should be clarified in the description. In that case, disregard my previous comment, my bad 🤔 |
# but there is a chance the response will fail and therefore | ||
# we should send the response | ||
if self.response._lock.is_responding(): | ||
async with self.response._lock: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure yet how to resolve the locking issue here, but this part should wait for the lock without setting _responded = True
on exit.
disnake/interactions/base.py
Outdated
type=defer_type.value, | ||
data=data or None, | ||
) | ||
self._responded = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is set by the lock on exit, it doesn't need to be set here as well
self._response = response | ||
self._lock = asyncio.Lock() | ||
|
||
async def __aenter__(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async def __aenter__(self): | |
async def __aenter__(self) -> None: |
self._lock.release() | ||
raise InteractionResponded(self._response._parent) | ||
|
||
async def __aexit__(self, exc_type, exc, tb): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async def __aexit__(self, exc_type, exc, tb): | |
async def __aexit__( | |
self, | |
exc_type: Optional[Type[BaseException]], | |
exc: Optional[BaseException], | |
tb: Optional[TracebackType], | |
) -> None: |
Needs types.TracebackType
import as well
return | ||
self._response._responded = True | ||
|
||
def is_responding(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def is_responding(self): | |
def is_responding(self) -> bool: |
098bd2c
to
81a39d3
Compare
81a39d3
to
bdb64c8
Compare
Summary
fixes #484
Checklist
task lint
task pyright