Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added Clear method for IMemoryCache to remove all entries. #44522

Closed
wants to merge 2 commits into from
Closed

Added Clear method for IMemoryCache to remove all entries. #44522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7a63068
Added Clear method for IMemoryCache to remove all entries.
ShreyasJejurkar Nov 11, 2020
9279367
Removed unwanted space added.
ShreyasJejurkar Nov 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...icrosoft.Extensions.Caching.Abstractions/ref/Microsoft.Extensions.Caching.Abstractions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ public partial interface IMemoryCache : System.IDisposable
Microsoft.Extensions.Caching.Memory.ICacheEntry CreateEntry(object key);
void Remove(object key);
bool TryGetValue(object key, out object value);
void Clear();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MCCshreyas - thanks for the contribution!

However, since this is public API we need to follow the API review process before accepting changes in this area. See the link at the top of this file: https://aka.ms/api-review process. We won't be able to merge this PR without first getting an API approval.

An issue with the currently proposed changes is that IMemoryCache is an interface, and adding more members to an interface is a binary breaking change. This type of change isn't acceptable in these libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So need to come up with new interface for a single method? 🤔

}
public static partial class MemoryCacheEntryExtensions
{
Expand Down
5 changes: 5 additions & 0 deletions src/libraries/Microsoft.Extensions.Caching.Abstractions/src/IMemoryCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,10 @@ public interface IMemoryCache : IDisposable
/// </summary>
/// <param name="key">An object identifying the entry.</param>
void Remove(object key);

/// <summary>
/// Removes all <see cref="ICacheEntry"/> object instances.
/// </summary>
void Clear();
}
}
1 change: 1 addition & 0 deletions src/libraries/Microsoft.Extensions.Caching.Memory/ref/Microsoft.Extensions.Caching.Memory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ protected virtual void Dispose(bool disposing) { }
~MemoryCache() { }
public void Remove(object key) { }
public bool TryGetValue(object key, out object result) { throw null; }
public void Clear() { }
}
public partial class MemoryCacheOptions : Microsoft.Extensions.Options.IOptions<Microsoft.Extensions.Caching.Memory.MemoryCacheOptions>
{
Expand Down
10 changes: 10 additions & 0 deletions src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,16 @@ public void Remove(object key)
StartScanForExpiredItems();
}

/// <inheritdoc />
public void Clear()
{
CheckDisposed();
if (!_entries.IsEmpty)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a race condition between IsEmpty and Clear. I'm not sure if it will be dangerous, though.
Is it possible to just call Clear (i.e. without the check for empty first)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, yes we can call directly Clear(), but I added that check just to avoid unnecessary calls to Clear().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be better if the concurrent dicitonary (CD) handles this case. I mean if Clear is called and the CD is empty, it just nops. Just as thought which is not finished...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the implementation of Clear() method, I did not see any check there, everytime it's initializing new table under the hood with given size! So I think it's better to have this check here.
But am still open for any changes if required!

Copy link
Contributor Author

@ShreyasJejurkar ShreyasJejurkar Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been fixed with my other PR - #44581

{
_entries.Clear();
}
}

private void RemoveEntry(CacheEntry entry)
{
if (EntriesCollection.Remove(new KeyValuePair<object, CacheEntry>(entry.Key, entry)))
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheServiceExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ public ICacheEntry CreateEntry(object key)
{
throw new NotImplementedException();
}

public void Clear() => throw new NotImplementedException();
}

private class TestDistributedCache : IDistributedCache
Expand Down
35 changes: 35 additions & 0 deletions src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,41 @@ public void RemoveRemoves()
Assert.Null(result);
}

[Fact]
public void Clear_ClearAllEntries()
{
var cache = CreateCache();

var obj = new object();
string key = "myKey";
var result = cache.Set(key, obj);
Assert.Same(obj, result);

var secondObject = new object();
string secondKey = "mySecondKey";
var secondResult = cache.Set(secondKey, secondObject);
Assert.Same(secondObject, secondResult);

cache.Clear();

result = cache.Get(key);
secondResult = cache.Get(secondKey);

Assert.Null(result);
Assert.Null(secondResult);
}

[Fact]
public void Clear_ThrowsExceptionIfCalledAfterDisposed()
{
var cache = CreateCache();
cache.Dispose();

Assert.Throws<ObjectDisposedException>(() => {
cache.Clear();
});
}

[Fact]
public void RemoveRemovesAndInvokesCallback()
{
Expand Down