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

Avoid websocket disconnects by sending pings #8

Merged
merged 2 commits into from
Oct 18, 2018
Merged

Avoid websocket disconnects by sending pings #8

merged 2 commits into from
Oct 18, 2018

Conversation

orottier
Copy link

@orottier orottier commented Feb 8, 2018

re issue botman/driver-slack#13, I've taken a bit different approach than the POC in the issue discussion, let me know what you think.
I'll run this over the weekend, let's see if it actually works :)

usage (botman):

$loop->addPeriodicTimer(60, function () use ($botman) {
    $timeout = 2.0;
    $botman->getClient()->checkConnection($timeout);
});

@mpociot
Copy link
Owner

mpociot commented Feb 20, 2018

@orottier Thank you for the PR. Did you have a chance to try this out?

@orottier
Copy link
Author

I did actually!

The websocket did not disconnect for 9 days, which is better than the typical 2/3 days (needs more statistics, I know). However the client stopped receiving pongs at some point but it did not trigger a reconnect. Looking at the code, I'm probably not catching the Timeout properly
->otherwise(function (Timer\TimeoutException $error) { has a wrong namespace

and since it's wrapped in another promise I think the Exception does not propagate or something. I'll fix that and add some more logging, to be continued.

@orottier
Copy link
Author

orottier commented Mar 9, 2018

It's been a wild ride Marcel

  • I borked one VM by var_dumping an error which filled the whole disk
  • I failed to reproduce the timeouts because apparently the inner loop interval is 5 seconds
  • I tried to trigger the timeout by sleeping before resolving promises but of course PHP is not async at all so the whole event loop sleeps
  • I fixed the inside of the TimeOut handler, somehow the error never propagated back up

The bot has consistently stopped receiving pongs after three/four days. I'm running again with the updated code and will let you know if it finally works ;)

@orottier
Copy link
Author

Good news! The websocket connection dropped over the weekend, but was detected by this code and successfully reconnected. :)

I guess we are good to go, but maybe you want to consider building the ping pong in the botman lib, instead of now asking the user to put in their own code

$loop->addPeriodicTimer(60, function () use ($botman) {
    $timeout = 2.0;
    $botman->getClient()->checkConnection($timeout);
});

@simensen
Copy link

simensen commented Jun 7, 2018

I could definitely use something like this. Are we close to being able to merge this? :)

@Carpenter0100
Copy link

@mpociot Is it possible to merge this? I have the same (issue).

usage (botman):

```
$loop->addPeriodicTimer(60, function () use ($botman) {
    $timeout = 2.0;
    $botman->getClient()->checkConnection($timeout);
});
```
@orottier
Copy link
Author

orottier commented Oct 9, 2018

Hi, I've rebased on the current master. @mpociot it would be great if you could merge and tag.

To summarize, this lets the slack client listen for pings. If it does not receive a pong before $timeout, the websocket will reconnect. It's up to the client to actually initiate the pings via

$loop->addPeriodicTimer(60, function () use ($botman) {
    $timeout = 2.0;
    $botman->getClient()->checkConnection($timeout);
});

@orottier orottier changed the title WIP: Try to avoid websocket disconnects by sending pings Avoid websocket disconnects by sending pings Oct 9, 2018
@mpociot mpociot merged commit 3c6eada into mpociot:master Oct 18, 2018
@mpociot
Copy link
Owner

mpociot commented Oct 18, 2018

Thank you very much for the work on this @orottier !

@guycalledseven
Copy link

Thank you! Can somebody please push this to packagist.org?
composer still sees v1.1 as latest composer show mpociot/slack-client --all:

versions : dev-master, * 1.1, 1.0, 0.3.4, 0.3.3, 0.3.2, 0.3.1, 0.3.0, 0.2.10, 0.2.9, 0.2.8, 0.2.7, 0.2.6, v0.2.5, v0.2.4, v0.2.3, v0.2.2, v0.2.1, v0.2.0, v0.1.1, v0.1.0

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

Successfully merging this pull request may close these issues.

5 participants