dotnet-eventsource
dotnet-eventsource copied to clipboard
launchdarkly
Retry field not used correctly
Even though the retry field is read it seems like it is not really used for the reconnection time.
Here the field is read to _retryDelay:
https://github.com/launchdarkly/dotnet-eventsource/blob/ee3c87574c4f694c0851e6c206b7e783a2ab11f7/src/LaunchDarkly.EventSource/EventSource.cs#L408-L414
But _retryDelay is not really used for the reconnection time:
https://github.com/launchdarkly/dotnet-eventsource/blob/ee3c87574c4f694c0851e6c206b7e783a2ab11f7/src/LaunchDarkly.EventSource/EventSource.cs#L201-L211
Instead it is only passed initially for construction of ExponentialBackoffWithDecorrelation:
https://github.com/launchdarkly/dotnet-eventsource/blob/ee3c87574c4f694c0851e6c206b7e783a2ab11f7/src/LaunchDarkly.EventSource/EventSource.cs#L99
The _minimumDelay in ExponentialBackoffWithDecorrelation is never updated
Having the SSE retry field combined with the "ExponentialBackOffWithDecorrelation", it is hard to tell what the expected behaviour is. If the server sends a retry duration I would expect that the reconnect time is never less then the received retry duration.
Even better: if there was a retry duration received from the server do not use the exponential back off strategy. Instead just use the retry time received from the sse server.
It does look like the current logic is incorrect, but I would have to think more on what the most logical behavior would be, because we can't look to the SSE specification in this case; the spec does not say anything at all about backoff/jitter, so it has no opinion on whether or not that should be applied to a server-sent retry value.
I am skeptical about entirely turning off the backoff/jitter behavior in this scenario. That would mean that any server that ever sends a retry: field is setting itself up for a thundering-herd situation the next time there's a network interruption, as every client would end up reconnecting at basically the same time.
Good point!
A simple solution for me would be to just recreate the ExponentialBackoffWithDecorrelation with the retry-field as new minimum.
I think it could be as easy as adding _backOff = new ExponentialBackoffWithDecorrelation(_retryDelay, _configuration.MaxRetryDelay); to line 413:
https://github.com/launchdarkly/dotnet-eventsource/blob/ee3c87574c4f694c0851e6c206b7e783a2ab11f7/src/LaunchDarkly.EventSource/EventSource.cs#L408-L414
The behaviour if the new minimum exceeds the old maximum also seems to be reasonable.
Yes, that's what I was thinking too. I think this was just an oversight. We're in process of writing a new 6.0 version of dotnet-eventsource that would not have this issue, but we should be able to do this fix in 5.x as well.
(However, please note that the way the ExponentialBackoffWithDecorrelation logic currently works, the base retry value is not really the minimum— because the random jitter is subtracted, rather than added.)