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

Addressing a few issues with #run_ping! (Fixes #231) #232

Merged
merged 4 commits into from
Oct 19, 2018

Conversation

RodneyU215
Copy link
Collaborator

Summary

This PR addresses several issues with the initial implementation (#226) of Slack::RealTime::Client#run_ping!; a method which is responsible for ensuring the Slack::RealTime::Client stays connected until Slack::RealTime::Client#stop! is called.

  • Slack::RealTime::Client#close will no longer assign nil to @socket because the reactor would still be running in the background if websocket_ping is set.
  • Slack::RealTime::Concurrency::Async::Socket#disconnect! now cancels all timers (via @reactor.cancel). This shuts down the websocket_ping timer.
  • I've removed the loop in Slack::RealTime::Client#run_ping! and now use on the builtin function Async::Reactor#every to run the ping on an interval.
  • I've added Slack::RealTime::Client#restart_sync which is responsible for obtaining a new websocket url and attempting to connect to it.
  • If the connection is closed and we've not called Slack::RealTime::Client#stop!; on the next Slack::RealTime::Client#run_ping! we'll attempt to restart the connection (via Slack::RealTime::Client#restart_sync)
  • If we're still not connected, after attempting to restart the connection (via Slack::RealTime::Client#restart_async), we reraise Slack::RealTime::Client::ClientNotStartedError
  • I've also added a new integration test to ensure it works as expected.

Requirements

@RodneyU215
Copy link
Collaborator Author

#231

@dblock
Copy link
Collaborator

dblock commented Oct 19, 2018

Looks good.

@dblock dblock merged commit 8702c27 into slack-ruby:master Oct 19, 2018
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.

2 participants