discord-php / DiscordPHP

An API to interact with the popular messaging app Discord
MIT License
992 stars 236 forks source link

HTTP does not pause WebSocket operations during pending reconnect #1228

Open valzargaming opened 4 months ago

valzargaming commented 4 months ago

Environment

Describe the bug The library continues to attempt operations on the WebSocket while a reconnect is pending, instead of waiting for the reconnection to complete. This can lead to failed requests and unexpected errors.

To Reproduce Attempt to perform any API calls after op code 1000 is received but before the socket is reconnected, which should happen automatically after 2 seconds.

Expected behavior The library should not attempt to perform any functions other than reconnect while the websocket is knowingly disconnected. Any other API-related promises that are created during this time should be deferred.

Additional context

[2024-05-19T18:13:03.191376+00:00] Civ13.WARNING: REQ POST channels/690025163634376738/messages failed: Connection to tls://discord.com:443 timed out after 60 seconds (ETIMEDOUT)
[2024-05-19T18:13:03.192807+00:00] Civ13.WARNING: websocket closed {"op":1000,"reason":""}
[2024-05-19T18:13:03.192853+00:00] Civ13.WARNING: reconnecting in 2 seconds
[2024-05-19T18:13:03.194160+00:00] Civ13.WARNING: REQ PATCH channels/1231988255470125117 failed: Connection to tls://discord.com:443 failed during TLS handshake: Connection lost during TLS handshake (ECONNRESET)
[2024-05-19T18:13:03.194254+00:00] Civ13.ERROR: Promise rejected with reason: `RuntimeException: Connection to tls://discord.com:443 failed during TLS handshake: Connection lost during TLS handshake (ECONNRESET) in /home/civ13/Civilizationbot/vendor/react/socket/src/SecureConnector.php:68
Stack trace:
#0 /home/civ13/Civilizationbot/vendor/react/promise/src/RejectedPromise.php(28): React\Socket\SecureConnector->React\Socket\{closure}()
#1 /home/civ13/Civilizationbot/vendor/react/socket/src/SecureConnector.php(64): React\Promise\RejectedPromise->then()
#2 /home/civ13/Civilizationbot/vendor/react/promise/src/FulfilledPromise.php(28): React\Socket\SecureConnector->React\Socket\{closure}()
#3 /home/civ13/Civilizationbot/vendor/react/promise/src/Promise.php(134): React\Promise\FulfilledPromise->then()
#4 /home/civ13/Civilizationbot/vendor/react/promise/src/Promise.php(168): React\Promise\Promise::React\Promise\{closure}()
#5 /home/civ13/Civilizationbot/vendor/react/promise/src/Promise.php(231): React\Promise\Promise->settle()
#6 /home/civ13/Civilizationbot/vendor/react/promise/src/FulfilledPromise.php(28): React\Promise\Promise::React\Promise\{closure}()
#7 /home/civ13/Civilizationbot/vendor/react/promise/src/Promise.php(134): React\Promise\FulfilledPromise->then()
#8 /home/civ13/Civilizationbot/vendor/react/promise/src/Promise.php(168): React\Promise\Promise::React\Promise\{closure}()
#9 /home/civ13/Civilizationbot/vendor/react/promise/src/Promise.php(231): React\Promise\Promise->settle()
#10 /home/civ13/Civilizationbot/vendor/react/promise/src/FulfilledPromise.php(42): React\Promise\Promise::React\Promise\{closure}()
#11 /home/civ13/Civilizationbot/vendor/react/promise/src/Promise.php(135): React\Promise\FulfilledPromise->done()
#12 /home/civ13/Civilizationbot/vendor/react/promise/src/Promise.php(168): React\Promise\Promise::React\Promise\{closure}()
#13 /home/civ13/Civilizationbot/vendor/react/promise/src/Promise.php(231): React\Promise\Promise->settle()
#14 /home/civ13/Civilizationbot/vendor/react/socket/src/TcpConnector.php(145): React\Promise\Promise::React\Promise\{closure}()
#15 /home/civ13/Civilizationbot/vendor/react/event-loop/src/StreamSelectLoop.php(254): React\Socket\TcpConnector->React\Socket\{closure}()
#16 /home/civ13/Civilizationbot/vendor/react/event-loop/src/StreamSelectLoop.php(213): React\EventLoop\StreamSelectLoop->waitForStreamActivity()
#17 /home/civ13/Civilizationbot/vendor/team-reflex/discord-php/src/Discord/Discord.php(1491): React\EventLoop\StreamSelectLoop->run()
#18 /home/civ13/Civilizationbot/src/Civ13.php(797): Discord\Discord->run()
#19 /home/civ13/Civilizationbot/bot.php(429): Civ13\Civ13->run()
#20 {main}'`
pro2s commented 4 months ago

is there a workaround for this issue? check the connection or something else before requesting.

valzargaming commented 4 months ago

is there a workaround for this issue? check the connection or something else before requesting.

I have been unable to come up with a feasible workaround due to the primary $discord object not making its WebSocket instance of $ws publicly accessible.

valzargaming commented 4 months ago

I've come up with one possible solution to this so far, however with Promises v2 the only way to effectively implement it is from userland. A workaround would involve implementing a middleware that handles the callback results for all ->then( statements and listening for the result. If the promise rejects with reason RuntimeException: Connection to tls://discord.com:443 timed out after 60 seconds (ETIMEDOUT) then the promise should be deferred and reattempted at a later time.

A promises v2 solution/workaround could look something like this:

$this->onFulfilledDefault = function ($result)
{
    $output = 'Promise resolved with type of: `' . gettype($result) . '`';
    if (is_object($result)) {
        $output .= ' and class of: `' . get_class($result) . '`';
        $output .= ' with properties: `' . implode('`, `', array_keys(get_object_vars($result))) . '`';
    }
    $this->logger->debug($output);
    return $result;
};
$this->onRejectedDefault = function ($reason): void
{
    $this->logger->error("Promise rejected with reason: `$reason'`");
    if ($reason === 'RuntimeException: Connection to tls://discord.com:443 timed out after 60 seconds (ETIMEDOUT)') {
        // Check for $reason here and recreate the promise to be reattempted after the connection has been restored
    }
};

/**
 * Chains a callback to be executed when the promise is fulfilled or rejected.
 *
 * @param PromiseInterface $promise The promise to chain with.
 * @param callable|null $onFulfilled The callback to execute when the promise is fulfilled. If null, the default callback will be used.
 * @param callable|null $onRejected The callback to execute when the promise is rejected. If null, the default callback will be used.
 * @return PromiseInterface The new promise that will be fulfilled or rejected based on the result of the callback.
 */
public function then(PromiseInterface $promise, ?callable $onFulfilled = null, ?callable $onRejected = null): PromiseInterface
{
    return $promise->then($onFulfilled ?? $this->onFulfilledDefault, $onRejected ?? $this->onRejectedDefault);
}

This will likely be possible to implement into the library in a non-hacky way whenever we can upgrade to Promises v3, after which we will be able to utilize set_rejection_handler.

In my case, I am simply listening for and logging the rejections, so I've included a section within my resolveOption() function to specifically validate what these default handlers should look like if passed into $options. Your preferred callbacks for promises may vary, but this may serve as a template if someone wants to do something similar.

$this->logger = $options['logger'];
$onFulfilledDefaultValid = false;
if (isset($options['onFulfilledDefault']) && is_callable($options['onFulfilledDefault'])) {
    if ($reflection = new ReflectionFunction($options['onFulfilledDefault']))
        if ($returnType = $reflection->getReturnType())
            if ($returnType->getName() !== 'void')
                { $this->onFulfilledDefault = $options['onFulfilledDefault']; $onFulfilledDefaultValid = true; }
}
if (! $onFulfilledDefaultValid) $this->onFulfilledDefault = function ($result) {
    $output = 'Promise resolved with type of: `' . gettype($result) . '`';
    if (is_object($result)) {
        $output .= ' and class of: `' . get_class($result) . '`';
        $output .= ' with properties: `' . implode('`, `', array_keys(get_object_vars($result))) . '`';
    }
    $this->logger->debug($output);
    return $result;
};
$onRejectedDefaultValid = false;
if (isset($options['onRejectedDefault']) && is_callable($options['onRejectedDefault'])) {
    if ($reflection = new ReflectionFunction($options['onRejectedDefault']))
        if ($returnType = $reflection->getReturnType())
            if ($returnType->getName() === 'void')
                { $this->onRejectedDefault = $options['onRejectedDefault']; $onRejectedDefaultValid = true; }
}
if (! $onRejectedDefaultValid) $this->onRejectedDefault = function ($reason): void
{
    $this->logger->error("Promise rejected with reason: `$reason'`");
    if ($reason === 'RuntimeException: Connection to tls://discord.com:443 timed out after 60 seconds (ETIMEDOUT)') {
        // Check for $reason here and recreate the promise to be reattempted after the connection has been restored
    }
};
SQKo commented 2 months ago

Ideally we should Pause the loop of the Discord-PHP-Http client, but since it's not practically possible with React Event loop, the solution to this is to rewrite the Discord-PHP Http queuing mechanism to allow requests to be paused and hold back in the bucket (like the Rate limit implementation) instead of forwarding the network issue as rejection and attempt to recreate the promise which could be indefinite