AddExpirationToken extension is not thread safe

[JunTaoLuo]

I've noticed that CacheEntryScopeExpirationTests.LinkContexts_AreThreadSafe test can occasionally fail when run repeatedly at https://github.com/aspnet/Caching/blob/dev/test/Microsoft.Extensions.Caching.Memory.Tests/CacheEntryScopeExpirationTests.cs#L475.

I will usually see one or two failures when repeating this test 50 times. I have reproduced this on Win10 x64 and Win10 x86.

cc @Tratcher

[JunTaoLuo]

Looks like the AddExpirationToken (

Caching/src/Microsoft.Extensions.Caching.Abstractions/CacheEntryExtensions.cs

Lines 29 to 40 in efbbdc5

	public static ICacheEntry AddExpirationToken(
	this ICacheEntry entry,
	IChangeToken expirationToken)
	{
	if (expirationToken == null)
	{
	throw new ArgumentNullException(nameof(expirationToken));
	}
	entry.ExpirationTokens.Add(expirationToken);
	return entry;
	}

) is not thread safe.

The issue arises when AddExpirationToken is called from different threads concurrently. Since they operate on the underlying ExpirationTokens, an IList, is not thread safe, some tokens may not be added.

A few possible fixes (need to make sure this doesn't introduce other races/deadlocks):

1. Use ICollection.SyncRoot of the ExpirationTokens collection for synchronization.
2. There's already a lock object for each entry we can use for synchronization though it's not exposed. We can move this extension to the interface instead.
3. MakeExpirationTokens a thread safe collection

Should review handling of expiration tokens for other potential races.

[Tratcher]

If there's going to be a lock it should be here:

Caching/src/Microsoft.Extensions.Caching.Memory/CacheEntry.cs

Line 325 in 21c850a

parent.AddExpirationToken(expirationToken);