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

HTTP does not pause WebSocket operations during pending reconnect #1228

Open
valzargaming opened this issue May 19, 2024 · 5 comments
Open
Labels

Comments

@valzargaming
Copy link
Member

Environment

  • PHP Version:
    • 8.3.6
  • DiscordPHP Version:
    • dev-master

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
Copy link

pro2s commented May 31, 2024

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

@valzargaming
Copy link
Member Author

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
Copy link
Member Author

valzargaming commented Jun 2, 2024

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

SQKo commented Jul 21, 2024

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

@valzargaming
Copy link
Member Author

I believe this issue should now 'softly' be considered as resolved. The exact details of how the client should respond to a timeout will likely vary from person to person, and as such anyone needing special handling should extend the Discord class and implement their own handling methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants