Looping through ConcurrentDictionary while modifying values

Question

I have a static ConcurrentDictionary in a static class. In the static constructor of the class, I call a private method via Task.Run to indefinitely loop through the dictionary and remove items that have expired, i.e. a significant amount of time has passed since they were added and as such need to be removed (this is determined by by inspecting a property on each CustomItem value of each item in the dictionary):

public static class ItemsHolder
{
    public static ConcurrentDictionary<Guid, CustomItem> Items = new ConcurrentDictionary<Guid, CustomItem>();

   // ...

    static ItemsHolder
    {
        Task.Run(() => DeleteExpired());
    }

    private static async Task DeleteExpired()
    {
        while (true)
        {
            // check if dictionary is empty and if yes, 
            // await Task.Delay a bit, etc - omitted for brevity

            foreach (var item in Items)
            {
                var guid = item.Key;
                var customItem = item.Value;

                if (customItem.ConditionToRemoveItemFromDictionaryHere)
                {

                    var removeItem = Items.TryRemove(guid, out _);

                    // ...
                }
            }
        }
    }
}

In other parts of the code (outside of this class), items are being added to this dictionary and removed based on other conditions. Effectively DeleteExpired() is there to clean out items that could not be removed from the dictionary (in other parts of the code, for XYZ reasons)

I see that DeleteExpired() keeps popping up in my metrics in tools such as dotTrace in R# as occupying CPU time. I know that iterating through the concurrent dictionary while other threads might be adding/removing is lock-free and should be safe to do, but I'm not sure whether this is the most effective way of doing so. Is there a more effective/efficient way of doing so?

Answer 1

I would have suggested to use a background task scheduler such as Quartz, as schedulers are especially made for that use case.

Create the Task for cleaning the dictionary:

public class CleanExpiredJob : IJob
{
    // Your ConcurrentDictionary value will be injected by the scheduler
    public ConcurrentDictionary<TKey, TValue> Items {private get; set;}
    public Task Execute(IJobExecutionContext context)
    {
        return Task.Run(() => {
            foreach (var item in Items)
            {
                var guid = item.Key;
                var customItem = item.Value;

                if (customItem.ConditionToRemoveItemFromDictionaryHere)
                {

                    var removeItem = Items.TryRemove(guid, out _);

                    // ...
                }
            }
        });
    }
}

Then, add the scheduler to your Application start:

    // Grab the Scheduler instance from the Factory
    NameValueCollection props = new NameValueCollection
    {
        { "quartz.serializer.type", "binary" }
    };
    var factory = new StdSchedulerFactory(props);
    var scheduler = await factory.GetScheduler();

    await scheduler.Start();
    var job = JobBuilder.Create<CleanExpiredJob>().Build();

    // Add your dictionary here
    // Notice: Keep the same name as the property in the job for injection
    job.JobDataMap["Items"] = Items;

    // Set the job to run every 10 seconds
    var trigger = TriggerBuilder.Create()
                    .StartNow()
                    .WithSimpleSchedule(x => x
                        .WithIntervalInSeconds(1)
                        .RepeatForever())
                    .Build();

    scheduler.ScheduleJob(job, trigger);

Answer 2

You're not waiting between each iteration, if the dictionary is not empty.

The while (true) makes a thread spin furiously and without rest (quite literally). You must absolutely wait between each check.

Without delay or sleep, one thread is going to continuously consume a whole core of your CPU (or at least it's going to try to do it), and this is the reason for the anomaly in performance.

I would write it as:

while(true) {
  Items.Where(pair => pair.Value.ConditionToRemoveItemFromDictionaryHere)
    .Select(pair => pair.Key)
    .ToList()
    .ForEach(guid => Items.TryRemove(guid, out _));

  // thread.sleep, task delay or whatever suits your needs
}