Add option to RedisCacheOptions to allow custom creation of IConnectionMultiplexer #32266
Conversation
Co-authored-by: Pranav K <[email protected]>
JunTaoLuo
added
area-runtime
community-contribution
| @@ -284,7 +299,7 @@ private byte[] GetAndRefresh(string key, bool getData) | |||
| return results[2]; | |||
| } | |||
|
|
|||
| return null; | |||
| return EmptyByteArray; | |||
There was a problem hiding this comment.
@Tratcher this seems like the only option without external changes but it's a breaking change? We don't really want to do this though since it's breaking the API contract and is a breaking change.
Alternatively, should we ask to change dotnet/runtime's definition to byte[]? The benefit there is that the interface definition would actually match the description. https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Caching.Abstractions/src/IDistributedCache.cs#L14-L19
There was a problem hiding this comment.
The API docs are clear that it returns null if the entry is not found. That's an important distinction because an empty entry and a missing entry can be semantically different.
Let's get the nullability fixed on IDistributedCache.
There was a problem hiding this comment.
If there is anything I can do to assist here I am game/able.
There was a problem hiding this comment.
I filed: dotnet/runtime#52148, we'll need that before we can enable nullability properly on this PR.
@JunTaoLuo what can I do to help out? I would really like to see this through. |
|
|
||
| private volatile ConnectionMultiplexer _connection; | ||
| private IDatabase _cache; | ||
| private volatile IConnectionMultiplexer? _connection; |
There was a problem hiding this comment.
Any idea why this is volatile?
| @@ -107,7 +108,7 @@ public void Set(string key, byte[] value, DistributedCacheEntryOptions options) | |||
|
|
|||
| var absoluteExpiration = GetAbsoluteExpiration(creationTime, options); | |||
|
|
|||
| var result = _cache.ScriptEvaluate(SetScript, new RedisKey[] { _instance + key }, | |||
| var result = _cache!.ScriptEvaluate(SetScript, new RedisKey[] { _instance + key }, | |||
There was a problem hiding this comment.
You can annotate Connect and ConnectAsync with [MemberNotNull(nameof(_cache))] to avoid the null forgiveness
There was a problem hiding this comment.
I had trouble applying MemberNotNull on ConnectAsync, we can chat about it when you are back.
|
@pranavkm It's unclear to me when dotnet/runtime will address dotnet/runtime#52148. Should we undo our nullability changes here and update the nullability of the APIs when we are unblocked? |
|
Yup. Let's not block on nullability if the underlying libraries haven't been updated as yet. |
ghost
added this to the
|
Thx @JunTaoLuo this was an enjoyable experience and I look forward to contributing more in the future! |
|
Hi @napalm684. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
amcasey
added
area-middleware
Replaces #31879.
FYI @napalm684 I recreated the PR since I couldn't push to your fork. We'll continue the PR here.