Why is the Lazy-pattern removed in the .NET Core sample?

[dehaaneric]

This issue is for a: (mark with an x)

- [ ] bug report -> please search issues before submitting
- [ ] feature request
- [X] documentation issue or request
- [ ] regression (a behavior that used to work and stopped in a new release)

OS and Version?

.NET Core 3.1 Web API on Azure App Services connecting to Azure REDIS.

Hi! Why is the Lazy implementation removed from the .NET Core code-sample?

EDIT, an additional question: while browsing the ASP.NET Core sample, I noticed the Redis commands are not executed async. Is that on purpose? I'm currently awaiting all commands (sets, get's etc).

The Microsoft Docs suggests we should use the Lazy pattern (and so I have currently in production). Trying to keep up to date with the latest findings and best practices I'm keeping an eye on this repo and I noticed this change. I'm not very familiar with the internals of Lazy, so maybe it's resolved and improved using a Semaphore implementation.

[philon-msft]

Hii @dehaaneric thank you for the questions. Lazy<> uses a lock internally that can lead to contention under high load. We recently converted away from Lazy to avoid potential problems in high-load environments. We're still working to clean up references to Lazy<> in docs like the one you found. For now, you can assume the quick start sample code shows the latest recommendations. Great point about the samples sending commands synchronously rather than async. That's definitely not our recommendation - async should be used whenever possible. I've opened a bug on our side to update samples appropriately.

[dehaaneric]

Thanks @philon-msft for that fast and clear answer. One last question (for now 😉): do you recommend to wrap all commands (StringSet, StringGet etc) within that BasicRetry-wrapper? Or only the GetDatabase() command.

[philon-msft]

Only the GetDatabase() call should be wrapped in BasicRetry.

To retry commands executed on the connection, we recommend using retry policies such as those provided by Pollly (http://www.thepollyproject.org/)

[dehaaneric]

Thanks @philon-msft. We got Polly for our SQL, I'll investigate how-to for REDIS. Closing.

[pmiddleton]

@philon-msft - I question the need for wrapping BasicRetryAsync around GetDatabase.

I looked at the code for GetDatabase in the StackExchange.Redis source. GetDatabase does not make network calls to the Redis server. It only returns a RedisDatabase object from an internal cache. Due to this it's impossible to ever end up in the first catch handler in BasicRetryAsync which would attempt a reconnect.

The majority of the time the only thing that the second ObjectDisposedException catch handler will achieve is putting the code in a tight expensive try/catch loop until RetryMaxAttempts is reached. This is because it's impossible for ForceReconnectAsync to ever be called due to the above issue. Unless by chance another thread happens to creates an new ConnectionMultiplexer at the exact moment you are in the ObjectDisposedException catch handler then the only thing ever achieved is rethrowing the ObjectDisposedException.

Also shouldn't the assignment of _connection in ForceReconnect be done via Interlocked.Exchange to insure all threads see the updated value.

[philon-msft]

@pmiddleton Thank you for the feedback - we'll look into this and get back to you.

[dehaaneric]

Reopening awaiting @philon-msft feedback

[amsoedal]

We have addressed the feedback and updated the samples to show how to use ForceReconnect w/ network calls, swapped sync for async overloads, as well as some other cleanup. The changes should go out to the public docs in the next couple of weeks, but for now you can get the updated code from this repo. Thanks for the suggestions, and feel free to let us know if you encounter any other issues.

[dehaaneric]

Thanks everyone. Really appreciate al the work in providing best practises, tips en tricks