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

Double Reconnect Test #61

Merged
merged 5 commits into from
Apr 17, 2017

Conversation

anacronw
Copy link
Contributor

@anacronw anacronw commented Jun 23, 2016

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.

@anacronw anacronw changed the title Bugfix/reconnect multiply tests Double Reconnect Test Jun 23, 2016
@anacronw anacronw mentioned this pull request Jun 23, 2016
@philstrong
Copy link
Contributor

thanks for adding the tests @Badunk

@philstrong
Copy link
Contributor

This fixes #57

@yinzer
Copy link

yinzer commented Nov 30, 2016

@aslakhellesoy any chance this can be reviewed?

@esalter
Copy link

esalter commented Jan 16, 2017

I ran into this as well. I'd love to see it merged. @aslakhellesoy any particular reason to hold this up?

@aslakhellesoy
Copy link
Contributor

@esalter see #69

@wKovacs64
Copy link
Contributor

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.

@rexxars
Copy link
Member

rexxars commented Apr 11, 2017

On Easter holidays at the moment, promise I'll look at this as soon as I get back!

Copy link
Contributor

@joeybaker joeybaker left a 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.

@rexxars rexxars merged commit 4b8338b into EventSource:master Apr 17, 2017
tricknotes added a commit to tricknotes/hubot-idobata that referenced this pull request May 28, 2017
EventSource/eventsource#61 is merged and
released as a new version of eventsource.
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.

8 participants