Make ConcurrentExpiringSet not leak the cleanup task for the period of delayBetweenCleanups #6576
Conversation
…f delayBetweenCleanups
|
Can one of the admins verify this patch? |
|
Raised lock free alternative that builds on top of this one #6577 |
| using System.Threading.Tasks; | ||
|
|
||
| sealed class ConcurrentExpiringSet<TKey> | ||
| { | ||
| readonly ConcurrentDictionary<TKey, DateTime> dictionary; | ||
| readonly ICollection<KeyValuePair<TKey, DateTime>> dictionaryAsCollection; |
There was a problem hiding this comment.
Will answer later
There was a problem hiding this comment.
ConcurrentDictionary implements the ICollection<KeyValuePair<TKey, TValue>> interface implicitly. By having a reference to that we can use the Remove(KeyValuePair<TKey, TValue> kvp) method which calls into TryRemoveInternal with matchValue set to true which then makes sure that the value matches the reference we get from looping over. This is basically the more accurate implementation that avoids removing a key when the value was updated in the meantime.
| readonly object cleanupSynObject = new object(); | ||
| CancellationTokenSource tokenSource = new CancellationTokenSource(); // doesn't need to be disposed because it doesn't own a timer |
There was a problem hiding this comment.
I believe CancellationTokenSource implements IDisposable and I think it might internally use a timer.
There was a problem hiding this comment.
It does. As far as I remember it only allocates disposable resources like timers when used with timeouts. Happy though to add
There was a problem hiding this comment.
See https://referencesource.microsoft.com/#mscorlib/system/threading/CancellationTokenSource.cs,554 that's why I did not implement the dispose call because we are not using the registrations or the timer path of the disposable
| { | ||
| this.tokenSource.Cancel(); | ||
| this.dictionary.Clear(); | ||
| this.tokenSource = new CancellationTokenSource(); |
There was a problem hiding this comment.
Should this be disposed of as it implements IDisposable?
There was a problem hiding this comment.
See comment above
| @@ -40,28 +52,38 @@ void ScheduleCleanup() | |||
| } | |||
|
|
|||
| this.cleanupScheduled = true; | |||
| Task.Run(async () => await this.CollectExpiredEntriesAsync().ConfigureAwait(false)); | |||
| _ = this.CollectExpiredEntriesAsync(token); | |||
There was a problem hiding this comment.
This will start a task that will do the collection without waiting for the cleanup to happen -- is this the design?
There was a problem hiding this comment.
Yes see previous implementation did a fire and forget Task.Run
| @@ -40,28 +52,38 @@ void ScheduleCleanup() | |||
| } | |||
|
|
|||
| this.cleanupScheduled = true; | |||
| Task.Run(async () => await this.CollectExpiredEntriesAsync().ConfigureAwait(false)); | |||
| _ = this.CollectExpiredEntriesAsync(token); | |||
There was a problem hiding this comment.
You don't need to assign the result of the method call if you don't plan to access it later (ie. no need for _ = )
There was a problem hiding this comment.
That is technically accurate. Though I consider it best practice to explicitly ignore not consumed tasks. Because if at any time this method would return a task the compiler would start exposing warning. I have to double check the VS diagbostics if they even detect dropped task in void methods
| } | ||
| } | ||
|
|
||
| this.ScheduleCleanup(); | ||
| this.ScheduleCleanup(token); |
There was a problem hiding this comment.
What happens if the token is canceled?
There was a problem hiding this comment.
It will cancel everything and not throw see try catch further down. Are you hinting that you'd like an realy exit before acquiring the lock?
| { | ||
| this.dictionary.TryRemove(key, out _); | ||
| this.dictionaryAsCollection.Remove(kvp); |
There was a problem hiding this comment.
I believe this will modify the collection while you are iterating over it, won't it?
Since both dictionary and dictionaryAsCollection point to the same object.
There was a problem hiding this comment.
sdk/servicebus/Microsoft.Azure.ServiceBus/tests/Primitives/ConcurrentExpiringSetTests.cs
Show resolved
Hide resolved
|
Replied to all the comments. Waiting for more input before I continue |
|
Closing in favor of #6577 |
Related to Azure Service Bus
Task.Delayif cleanup was scheduled to not leak the task for the duration of the timeout.Keysaccess that locks the whole concurrent dictionary