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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

onerandomusername
Copy link
Member

Summary

fixes #484

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running task lint
    • I have type-checked the code by running task pyright
  • 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 onerandomusername added the bug Something isn't working label Apr 27, 2022
@onerandomusername onerandomusername added the s: needs review Issue/PR is awaiting reviews label Apr 27, 2022
@onerandomusername onerandomusername added this to the disnake v2.6 milestone May 5, 2022
@onerandomusername
Copy link
Member Author

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

@onerandomusername onerandomusername added the p: high High priority label Jun 30, 2022
@shiftinv
Copy link
Member

shiftinv commented Jul 4, 2022

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

@onerandomusername
Copy link
Member Author

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

@shiftinv
Copy link
Member

shiftinv commented Jul 5, 2022

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 🤔

# 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:
Copy link
Member

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.

type=defer_type.value,
data=data or None,
)
self._responded = True
Copy link
Member

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):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def is_responding(self):
def is_responding(self) -> bool:

@shiftinv shiftinv modified the milestones: disnake v2.6, disnake v2.7 Sep 1, 2022
@onerandomusername onerandomusername force-pushed the fix-interaction-race-conditions branch 3 times, most recently from 098bd2c to 81a39d3 Compare October 1, 2022 18:28
@onerandomusername onerandomusername force-pushed the fix-interaction-race-conditions branch from 81a39d3 to bdb64c8 Compare October 4, 2022 05:01
@onerandomusername onerandomusername added p: medium Medium priority and removed p: high High priority labels Oct 25, 2022
@shiftinv shiftinv removed this from the disnake v2.8 milestone Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p: medium Medium priority s: needs review Issue/PR is awaiting reviews
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

interactions: possible race condition when responding
2 participants