Retry field not used correctly

[thorstenfleischmann]

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:

dotnet-eventsource/src/LaunchDarkly.EventSource/EventSource.cs

Lines 408 to 414 in ee3c875

	else if (result.IsRetryField)
	{
	if (long.TryParse(result.GetValueAsString(), out var retry))
	{
	_retryDelay = TimeSpan.FromMilliseconds(retry);
	}
	}

But _retryDelay is not really used for the reconnection time:

dotnet-eventsource/src/LaunchDarkly.EventSource/EventSource.cs

Lines 201 to 211 in ee3c875

	private async Task MaybeWaitWithBackOff() {
	if (_retryDelay.TotalMilliseconds > 0)
	{
	TimeSpan sleepTime = _backOff.GetNextBackOff();
	if (sleepTime.TotalMilliseconds > 0) {
	_logger.Info("Waiting {0} milliseconds before reconnecting...", sleepTime.TotalMilliseconds);
	BackOffDelay = sleepTime;
	await Task.Delay(sleepTime);
	}
	}
	}

Instead it is only passed initially for construction of ExponentialBackoffWithDecorrelation:

dotnet-eventsource/src/LaunchDarkly.EventSource/EventSource.cs

Line 99 in ee3c875

_backOff = new ExponentialBackoffWithDecorrelation(_retryDelay, _configuration.MaxRetryDelay);

The _minimumDelay in ExponentialBackoffWithDecorrelation is never updated

[thorstenfleischmann]

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.

[eli-darkly]

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.

[eli-darkly]

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.

[thorstenfleischmann]

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:

dotnet-eventsource/src/LaunchDarkly.EventSource/EventSource.cs

Lines 408 to 414 in ee3c875

	else if (result.IsRetryField)
	{
	if (long.TryParse(result.GetValueAsString(), out var retry))
	{
	_retryDelay = TimeSpan.FromMilliseconds(retry);
	}
	}

The behaviour if the new minimum exceeds the old maximum also seems to be reasonable.

[eli-darkly]

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.)