fix: add a lock around interaction responses to fix race conditions in case multiple responses are made at once
Summary
fixes #484
Checklist
- [x] If code changes were made, then they have been tested
- [ ] I have updated the documentation to reflect the changes
- [x] I have formatted the code properly by running
task lint - [x] I have type-checked the code by running
task pyright
- [x] This PR fixes an issue
- [ ] This PR adds something new (e.g. new method or parameters)
- [ ] This PR is a breaking change (e.g. methods or parameters removed/renamed)
- [ ] This PR is not a code change (e.g. documentation, README, ...)
@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. There might be situations where you don't want/need this behavior, and we're not doing any locking for other endpoints either, e.g. trying to delete a channel and sending a message there at the same time. Since this is a fairly superficial change (in terms of lib interfaces), at least that particular race condition in the linked issue #484 could easily be resolved by wrapping the response part in a lock instead:
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 Interaction.send should not be erroring with an InteractionAlreadyResponded exception, as that method is partially to avoid those errors.
A different solution would be try/except and use the followup if we catch an already responded error in Interaction.send
The issue I see is that
Interaction.sendshould not be erroring with anInteractionAlreadyRespondedexception, as that method is partially to avoid those errors.
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.
A different solution would be try/except and use the followup if we catch an already responded error in
Interaction.send
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 🤔