eventsource
eventsource copied to clipboard
Fixed duplicates reconnect mentionned in issue #89
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.
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.
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!
@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 https://github.com/tigertext/eventsource/commit/e39460e7b852a0b286d7b752efb8f0bcb04f1057#diff-bd6ddbbcfe0037b5a08d70a3b5f4930c , it dose not really solve the duplicate reconnection issue, check my comment HERE