Remove IDisposable from ICacheEntry

[Tratcher]

https://github.com/aspnet/Caching/blob/65228be1d9aba6f2c360553868010f18706ee302/src/Microsoft.Extensions.Caching.Abstractions/ICacheEntry.cs#L13

IDisposable recommends a using pattern as follows, where Dispose commits the entry to the cache:

using (var entry = MemoryCache.CreateEntry(cacheKey))
{
  // The result is processed inside an entry
  // such that the tokens are inherited.
  entry.Value = CreateValue(params);
  entry.SetOptions(options); 
}

However, if CreateValue throws then the entry is still committed to the cache as null. Thus the using pattern should not be used with ICacheEntry, and we should remove IDisposable to prevent it. Replace it with a Commit method.

aspnet/Mvc#6522

[muratg]

Looks like this was worked around in MVC. I don't expect we'll do a breaking change here in 3.0.

[gravity00]

Hi @muratg is there any plans to proceed with the breaking change?

[Tratcher]

@anurse

[analogrelay]

As I understand it, to "fix" this we'd be introducing a breaking change. It's too late to do that kind of thing in 3.0, so there is no change planned here in 3.0.

[gravity00]

Sorry @anurse, I wasn't completely explicit.
My question was for a major after v3 because, since this issue is closed, was it added to a backlog for contracts revision?

[analogrelay]

No, it was closed back in January. If you'd like to request this change be reconsidered, we can do that. What's the scenario that's broken for you right now?

[gravity00]

Nothing is currently broken per se, since you can work around even in older versions (just create the value before requesting the cache entry), but is about consistency when implementing the disposable pattern, like @Tratcher said, while also being explicit that a new entry have been added to the cache.

The thing is, you request an entry from the cache and not create one by yourself, so it is not clear that disposing the entry will also add it to the cache (as far as what concerns the developer, it is implementing IDisposable to release resources) and if an exception is thrown inside the using it will still save the entry in whatever state it may be.

To ensure some kind of atomic behavior during the entry setup, adding a commit method would made the approach more explicit and less prone to misinterpretations. The simple fact comments to this behavior must be added to the extensions methods or it may be forgotten are a clear indication of an implementation problem.

The IDistributedCache doesn't have this problem because it only exposes methods that enable an atomic behavior (arguments are the key, value and entry options).

[analogrelay]

The thing is, you request an entry from the cache and not create one by yourself, so it is not clear that disposing the entry will also add it to the cache

I do agree this feels odd and counter-intuitive. I don't have background on why this pattern was used, but it feels confusing to me. So I'm with you here :).

The main thing we think about when considering breaking changes (as with any changes) is if the benefits outweigh the costs. The cost for a breaking change starts out fairly high since users are forced to react if they upgrade. Generally we do this by marking an API as obsolete in one version (so that users get a warning, but the API still works) and then removing it in the next version. It seems like that would be a reasonable thing to consider in the 5.0 time frame (which is the next release open for this kind of change).

I'll open this up and put it on the backlog for consideration as we do 5.0 planning.

[JakenVeina]

Perhaps the shorter-term solution is to implement an alternate way to add to the cache, and go ahead and mark the current versions of ICacheEntry and .CreateEntry() as deprecated?