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

Automatic connection reconnecting #385

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Automatic connection reconnecting #385

wants to merge 13 commits into from

Conversation

macjuul
Copy link
Contributor

@macjuul macjuul commented Nov 25, 2024

Thank you for submitting this pull request! We appreciate you spending the time to work on these changes.

What is the motivation?

The JavaScript SDK currently does not offer any functionality for automatically recovering from an unexpected disconnect. Additionally, this functionality is desired by many users and will greatly benefit from an internal standardised implementation.

What does this change do?

Implements a new reconnect option passed to connect() which takes a boolean or a reconnect options object. The reconnect options supports configuring exponential backoff (enabled by default) including random jittering.

Reconnect options

  • enabled: Whether the SDK will attempt to reconnect after disconnecting (default true)
  • attempts: The maximum amount of connection attempts (default 5)
  • retryDelay: The base delay to wait between each attempt (default 1000)
  • retryDelayMax: The maximum amount of time to wait between attempts (default 60000)
  • retryDelayMultiplier: The amount to multiply each delay by (default 2)
  • retryDelayJitter: A percentage to randomly offset each delay by (default 0.1)

Notes

  • Pass any negative number to attempts in order to continue reconnecting infinitely
  • If you require dynamic authentication values for reconnecting it is recommended to use the prepare function, which will be invoked for each successful reconnect
  • Executed queries will continue to await until the connection is re-established, or reject after the maximum amount of attempts have been reached
  • Closing a connection manually using close() will not trigger a reconnect

What is your testing strategy?

TBD

Is this related to any issues?

#203

Have you read the Contributing Guidelines?

@macjuul macjuul mentioned this pull request Nov 25, 2024
1 task
src/surreal.ts Outdated Show resolved Hide resolved
@ntorrey
Copy link

ntorrey commented Nov 25, 2024

Does the 'pinger' logic need to be updated? I believe that is what was throwing errors before. If it tried to ping the websocket connection when the connection was down it would make the application crash.

@macjuul
Copy link
Contributor Author

macjuul commented Nov 26, 2024

@ntorrey The pinger should already be cleaning up correctly, however since the pinger exists to keep the socket open, I've made it ignore potential errors since we don't handle these anyways. This might resolve the error itself, however doesn't explain why socket closing isn't handled correctly

@ntorrey
Copy link

ntorrey commented Nov 27, 2024

Because I like living life on the edge. I've already deployed these changes to production. 🤣
So far it's working great!
I only have around 10 - 20 users and it's not a mission critical application though, so it's not a huge deal.

@ntorrey
Copy link

ntorrey commented Nov 27, 2024

Not sure if this is the PR to address this, and it could be that I'm doing something wrong, but one thing I discovered while testing this (I'm aware that this is obviously not production-ready code) is that when on a pwa mobile app, when I put put the app into the background and try to come back to it it does not remain connected or reconnect to the database, but closing and reopening the app works fine. Perhaps I need to do something with the service worker setup. Since it is on mobile I wouldn't know how to debug this. Working fine everywhere else though.

@LucyEgan
Copy link

@ntorrey i notice this same behaviour with our reconnection logic(which is the same ish implementation as this except outside of the sdk) It will also happens on desktop between pc sleeps/backgrounded browser tabs. And i would assume the same with mobile(outside of pwa) and general backgroundedness of things. Im not sure of a reliable way to test/reproduce it so that it can be properly implemented to handle this as of yet

@ntorrey
Copy link

ntorrey commented Nov 27, 2024

@LucyEgan Thanks!
So I was able to peek into the console of the mobile app and I get this error:
ERROR NoActiveSocket: No socket is currently connected to a SurrealDB instance. Please call the .connect() method first!
But Claude AI came to the rescue. There is an event that fires when the app is brought back from the background. I just need to see the best way to leverage this and if it solves the issue.

document.addEventListener('visibilitychange', () => {
  if (document.visibilityState === 'visible') {
    // The page has become visible again (app brought back from background)
    // connect() logic goes here
  }
});

@LucyEgan
Copy link

@ntorrey yeh thats what i was thinking, but beware that will get triggered each time you view the pwa/tab/window again (good old ai suggesting good enough stuff 🤣)
I would like to see the sdk handle this if possible to not give you a NoActiveSocket when it expects to be in a connected/reconnecting state, but hang the queries(in order) until the reconnection/persistence is complete and only fail when its actually not connected.

I would probably do that bit my side if its not in the sdk, but i feel like all these extra cases should be handled otherwise you end up implementing the rest of connection management outside anyway

@ntorrey
Copy link

ntorrey commented Nov 28, 2024

Sorry for spamming this PR, but hopefully this is helpful. Another case that has happened to me is on the server. When my fly.io instance is suspended and restarts I get the same error.

@triracle97
Copy link

triracle97 commented Dec 19, 2024

What happens if the ping fails to ping the server. I'm encountering a case where the connection status is connected but I can't send anything to the database (ws requests keep hanging). In this case the only solution is to restart the whole web tab and create a new connection.
I'm on 1.0.0-alpha.9, but I think the new version still has this issue, I'm encountering it using surrealist also - I opened surrealist and then put my laptop in sleep mode, when I reopened it, the connection status was connected but I couldn't do anything, the same thing happened to my browser tab.

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.

6 participants