disnake icon indicating copy to clipboard operation
disnake copied to clipboard

fix: add a lock around interaction responses to fix race conditions in case multiple responses are made at once

Open onerandomusername opened this issue 3 years ago • 4 comments

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, ...)

onerandomusername avatar Apr 27 '22 02:04 onerandomusername

@shiftinv Hi, would you please consider reviewing this pr?

onerandomusername avatar Jun 25 '22 22:06 onerandomusername

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}")

shiftinv avatar Jul 04 '22 10:07 shiftinv

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

onerandomusername avatar Jul 04 '22 20:07 onerandomusername

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.

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 🤔

shiftinv avatar Jul 05 '22 14:07 shiftinv