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

Retrying with exponential back-off and randomisation #80

Closed
enisdenjo opened this issue Nov 27, 2020 · 7 comments · Fixed by #84
Closed

Retrying with exponential back-off and randomisation #80

enisdenjo opened this issue Nov 27, 2020 · 7 comments · Fixed by #84
Labels
enhancement New feature or request good first issue Issue tackles a good aspect released Has been released and published

Comments

@enisdenjo
Copy link
Owner

Story

As a client

I want to use exp back-off and randomisation on retries

So that I prevent the "thundering herd" problem and avoid DDOSing the server when it restarts

Acceptance criteria

  • Client uses the exp back-off for retries
  • Client randomises the initial timeout on retries
@Shahor
Copy link

Shahor commented Dec 8, 2020

Hey @enisdenjo 👋

Great on work on the graphql-ws library!

I'm fighting a bit with this issue this morning, I'm seeing a double problem here:

  • When my server restarts, the client waits retryTimeout before making a new connection, but if the server is not started yet when the retryTimeout is done, it'll just loop retryAttempts times without respecting the retryTimeout. What can I do to make sure retryTimeout (or retryStrategy(tryNumber: number)) is properly awaited?

The scenario down below is happening with the following configuration:

return new SubscriptionLink({
      url: subscriptionsEndpoint,
      retryTimeout: 30e3,
      on: { /* event handlers */ }
})

image

As you can see, the retryTimeout is not used for the socket reconnection attempts.

@enisdenjo
Copy link
Owner Author

Hi @Shahor, thanks for reporting this - it is most probably a bug! I have a hunch about what causes it, will fix it probably together with the upcoming PR.

Regarding the retryStrategy in #84 - I like the idea a lot! Will include this in the PR, thanks for the suggestion. 👍

@Shahor
Copy link

Shahor commented Dec 9, 2020

Ah, that is great news thanks for considering it 🙇

I'd be happy to test this in my environment if you want once ready.

@Shahor
Copy link

Shahor commented Dec 9, 2020

@enisdenjo I am very sorry for being pushy with this, is there any chance you'd have an ETA for at least the buggy part?

I'm happy to help if needed to make this move forward :) (It's blocking us a bit as we're switching to graphql-ws because of a bug that it fixes compared to subscriptions-transport-ws)

Just let me know if I can help you on this :)

@enisdenjo
Copy link
Owner Author

No worries, I can understand!

I'll try pushing the fix later today (am currently swamped with my day job, as soon as I am done - I'll come back to the PR).

@enisdenjo
Copy link
Owner Author

🎉 This issue has been resolved in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@enisdenjo enisdenjo added the released Has been released and published label Dec 9, 2020
@Shahor
Copy link

Shahor commented Dec 9, 2020

@enisdenjo You rock 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Issue tackles a good aspect released Has been released and published
Projects
None yet
2 participants