search

discord-php / DiscordPHP-Http

Asynchronous HTTP client used for communication with the Discord REST API.
MIT License
21 stars 9 forks source link

Fix deferred reject #10

Closed jocafamaka closed 2 years ago

jocafamaka commented 2 years ago

This PR aims to correct the lack of deferred rejection, when a non-HTTP error occurs.

When the error does not occur during the request, but before it.

This behavior was causing a block in the request execution queue, perceived due to the incorrect behavior of the websocket reconnection attempts, causing a bot inertia state, see: The bot shuts down on its own. #534

As this package is the basis for all other requests, I am creating this PR as a draft, so that it can be validated.

valzargaming commented 2 years ago

This looks good to me and is very similar to what Yasmin did, except it actually threw an 'error' event that could be listened to. It might be worthwhile to consider adding something like that to DPHP in the future.

SQKo commented 2 years ago

Some notes from me, I'd say this is a temporary work around since it might be related to this issue: https://github.com/reactphp/promise/issues/87

Of course we can't wait the fix for now since it's gonna be for next major version of ReactPHP promise. Then it is something to be considered in next major version of DiscordPHP as well.

We can say this is perfect for one request in queue, but I doubt it's gonna make another issue if the requests in queue have to be done in concurrent.

Copying from my words on Discord:

It is a recursive function, since is also dealing with the concurrency and queue numbers, I need to make sure this wont break the queue count or bypass the internal rate limiting it is something that i need to learn deeper But I'm sure David has idea the first concurrent request, makes the first deferred, and the next request after recursion pass that deferred until the queue ends, thats what I've understood so far Indeed the fix is to reject that deferred so the exception is catch, but how it is gonna affect with the concurent requests and queue seems an undefined behavior to me we can take guess that fix is only perfect for one request I'll need code to reproduce the issue, not at random times, so i can reproduce it in multiple concurrent requests

So right now, I will push this to version 9.0.10 but not bump the required version in DiscordPHP's composer.json, people who will do composer update will get still get this fix, but if they face some unexpected behavior from this, they can lock/downgrade back to v9.0.9.