-
Notifications
You must be signed in to change notification settings - Fork 252
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
Double Reconnect Test #61
Double Reconnect Test #61
Conversation
thanks for adding the tests @Badunk |
This fixes #57 |
@aslakhellesoy any chance this can be reviewed? |
I ran into this as well. I'd love to see it merged. @aslakhellesoy any particular reason to hold this up? |
Got time to review this, @rexxars? Can't speak to the test implementation, but the fix itself has been valuable enough that I've had to run philstrong's fork. |
On Easter holidays at the moment, promise I'll look at this as soon as I get back! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been using something like this for a while in prod.
EventSource/eventsource#61 is merged and released as a new version of eventsource.
Here's a PR with both @philstrong work and my test to expose the bug.
The test's approach is for the server to disconnect the first connection immediately. Subsequent connections are allowed to sit around. You would expect 2 connections (1 initial, 1 retry) within the timeframe of the test, however there are actually 3 that are made (2 retries).
The test might not be to your liking as it waits 1500 seconds before succeeding. This threshold basically has to exceed the 1,000 of the
reconnectInterval
so that the client may issue a 2nd (and 3rd) reconnect.