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

Fixed duplicates reconnect mentionned in issue #89 #135

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Hedgestock
Copy link

@Hedgestock Hedgestock commented May 4, 2020

In some cases the server going down can lead to multiple triggers of the onConnectionClosed() method, resulting in multiple reconnection timeouts being created. I fixed this problem by only allowing one timeout to be running at a time.

@Hedgestock Hedgestock marked this pull request as ready for review May 4, 2020 14:04
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.

Thanks for working on this! I left you a few comments, but I'd like to talk about the general strategy for solving this. If you look at the issue you referenced, #89, it talks about tigertext@e39460e#diff-bd6ddbbcfe0037b5a08d70a3b5f4930c Did you look at that strategy?

Also, we'd need tests to make sure we don't regress on this issue.

lib/eventsource.js Outdated Show resolved Hide resolved
lib/eventsource.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Hedgestock
Copy link
Author

Thanks for working on this! I left you a few comments, but I'd like to talk about the general strategy for solving this. If you look at the issue you referenced, #89, it talks about tigertext@e39460e#diff-bd6ddbbcfe0037b5a08d70a3b5f4930c Did you look at that strategy?

Also, we'd need tests to make sure we don't regress on this issue.

I looked at what they did but for some reason it did not work for me, I'm using a C# server and even with their fix I still managed to get duplicate reconnections attempts when killing the server or unplugging machine running the server. I won't pretend to fully understand what's happening in this lib but I'm pretty confident my fix works (well at least for my use case). And I guess that's why you mentioned adding tests to prevent regression, however I have no Idea on how to do that in practice yet.

@joeybaker
Copy link
Contributor

The reason I suspect that other commit would be a good fix is that it's effectively doing the same thing as your timeout fix, but still leaves us open to the possibility that there's a condition where something is re-marking the connection as open when it shouldn't. If that's true, that's the root cause of the problem and we should fix that!

btw, if you've got an issue with nyc that's worth opening an issue on!

@icy-fish
Copy link
Contributor

icy-fish commented May 22, 2020

@Hedgestock @joeybaker I think this PR #125 has already fixed the issue mentioned here, but this PR has reverted that fix, maybe by mistake when solving conflictes, please check.

As for tigertext@e39460e#diff-bd6ddbbcfe0037b5a08d70a3b5f4930c , it dose not really solve the duplicate reconnection issue, check my comment HERE

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.

3 participants