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

Handle client reconnects #236

Closed
gregdhill opened this issue Mar 10, 2021 · 4 comments
Closed

Handle client reconnects #236

gregdhill opened this issue Mar 10, 2021 · 4 comments

Comments

@gregdhill
Copy link
Contributor

gregdhill commented Mar 10, 2021

Currently if a request fails for any reason (particularly in the websocket client) the expected recourse is to end the background task and propagate to the caller. I'm not yet entirely sure how it would look, but it might be possible to handle reconnects in this library. Or perhaps this is the responsibility of the caller...

@niklasad1
Copy link
Member

niklasad1 commented Mar 10, 2021

I think we should tackle this as follows for more user-friendly experience:

  1. Provide a separate error variant that states Error::RestartNeeded(Reason)
  2. Provide a restart method that users can call explicitly

Thoughts?

//cc @maciejhirsz

@maciejhirsz
Copy link
Contributor

I'm of two minds here. I think having hard errors on disconnects and having the caller handle reconnecting is the most sensible default. On the other hand the client is a pretty high level of abstraction, so having it not just handle reconnects, but also something like a pool of keep-alive connections in case of the HTTP client might be sensible.

The only drawback I see here is that once I do anything automated we need to make sure it's properly configurable, e.g. most sensible reconnect strategy for most use-cases is exponentially growing timer between attempts with some ceiling, but that ceiling, the initial time, and whether or not it should grow over time at all should then all be options.

@dvdplm
Copy link
Contributor

dvdplm commented Mar 12, 2021

I think having hard errors on disconnects and having the caller handle reconnecting is the most sensible default.

I think this is the way to go. I think we need to get the basics right first and possibly offer refinements such as connection pools and configurable reconnect strategies further down the line if/when we see a concrete need.
If we design a good API, handling reconnects should be fairly easy to do for upstream projects.

@niklasad1
Copy link
Member

Closing this because we introduced hard errors for caller to handle if the connection gets reset:

  1. A error variant Error::RestartNeeded when the connection gets reset, if you make a method call or subscription you will get that to indicate that connection was reset.
  2. Client::is_closed non blocking API to poll whether the connection is still open.

I guess maybe we could maybe improve the API to register callback (future to resolve when the background task gets terminated), if polling or matching on Error::RestartNeeded is problematic.

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

No branches or pull requests

4 participants