Fix race condition

[Mihitoko]

Summary

Fixes a race condition in interactions.py when two responses are made at the same time.
This PR fixes #963

Checklist

• If code changes were made then they have been tested.
  • I have updated the documentation to reflect the changes.
• If type: ignore comments were used, a comment is also left explaining why
• 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, typehinting, examples, ...)

[krittick]

I feel like automatically sending a followup when an exception is encountered isn't the right way to handle this. I also don't think this is an issue with the library itself, but rather just how asyncio.create_task() works.

Using the same example in the original issue but replacing the double asyncio.create_task(respond()) with double await respond() does not cause the issue.

[Mihitoko]

I feel like automatically sending a followup when an exception is encountered isn't the right way to handle this. I also don't think this is an issue with the library itself, but rather just how asyncio.create_task() works.

Sending a followup when the interaction is already responded was also implemented before i made changes.
The exeption catch only ensures that we followup when a response was made while we executed cureent response.

Using the same example in the original issue but replacing the double asyncio.create_task(respond()) with double await respond() does not cause the issue.

It was just an example.
This happens a lot when you implement auto defering by yourself.
And the task to defer the interaction executes at the same time as the response.
This issue is pretty niche but a race condition that can lead to unexpected behavior should still be adressed in my opinion.

[krittick]

Can you provide an example that doesn't use create_task() then?

[Mihitoko]

Can you provide an example that doesn't use create_task() then?

I think you understood me wrong.
I know its that behavior of create task that enables this exception.
If you await the task it will not happen.
So no i cant

My point was that there a good reasons for putting a response in a task and not awaiting it.
So from my point of view we should ensure that no race condition is happening.

[BadWolf1023]

Can you provide an example that doesn't use create_task() then?

Why? That’s a basic usage of asyncio. But for the sake of the question, you could replicate this behavior without explicitly using create_task in your specific code in several different ways. For example, if someone ran a slash command and your slash command code made a reference to another ApplicationContext, calling .respond() on that object.

The problem is here:
3dcd834#diff-d40bf8033607a75e12d53f161d4983e9c1dff8eaa62eba83e947874d7812f5fdL792
When you call await in this line, you allow the asyncio loop to run a different scheduled (or awaited) coroutine which may also call .respond on that same object which could finish before this line’s coroutine is run by the asyncio loop.

[Mihitoko]

In my opinion catching the InteractionResponded exception is the best way to go here.
There is nothing hacky about catching an exception that is expected to be risen.

A soloution that might also work is

Add this method to InteractionResponse

# Not sure about the name tho, maybe change it
async def respond_dynamic(self, *args, **kwargs):
    try:
        await self.send_message(*args, **kwargs)
    except InteractionResponded:
        # get followup webhook from parent
        await self._parent.followup.send(*args, **kwargs)

This still invoves the error catch but is a lot cleaner then my aproach catching the error in the context object.

ApplicationContext

Just retrun the method above in ApplicationContext.respond

What are your thoughts about this aproach ?

[Vondyy]

Regarding #1041 I did download Mihitoko fork and it still happens, so my issue lies elsewhere I'm pre sure. So, don't think application did not respond is not tied to this.

[Mihitoko]

Regarding #1041 I did download Mihitoko fork and it still happens, so my issue lies elsewhere I'm pre sure. So, don't think application did not respond is not tied to this.

Did you download fix_race_condtition branch?
The fix is not on master on my fork.
Although Im not sure if that's really the problem in your issue.

[Vondyy]

I used pip install git+https://github.com/Mihitoko/pycord.git and I still get the issue. I made sure I did pip uninstall py-cord first

[VincentRPS]

I used pip install git+https://github.com/Mihitoko/pycord.git and I still get the issue. I made sure I did pip uninstall py-cord first

You need to add @fix_race_condition to that link.

[BobDotCom]

Merge conflicts here, please resolve.

[Mihitoko]

Merge conflicts here, please resolve.

Done.

[TurnrDev]

You forgot a return and you might as well refactor respond to just be an async function. It being a property was always a mistake

[krittick]

You forgot a return and you might as well refactor respond to just be an async function. It being a property was always a mistake

The core devs have been discussing this internally and this is actually part of our planned approach to tackle this.

[Dorukyum]

These suggestions should turn this into an async function.

stale

[krittick]

LGTM now

[Dorukyum]

Minor documentation/styling changes

Resolved