[Bug]: ExpireAfter might not expire the most recent item.

[alex-borzenko]

Describe the bug 🐞

When using ExpireAfter without polling but scheduling expiry action for each item, the most recent item might get stuck, if the scheduler fires a microsecond earlier then that item's due time. I had to work around it by wrapping the scheduler and adding a millisecond to the dueTime (works for my "slow" trading app :) ).

Step to reproduce

/// <summary>
/// This test demonstrates the problem with ExpireAfter
/// </summary>
/// <param name="count"></param>
/// <param name="delay"></param>
[TestCase(10,10)]
public async Task TestExpireAfter(int count, int delay)
{
    var source = new SourceCache<int, int>(x => x);

    source
        .ExpireAfter(_ => TimeSpan.FromMilliseconds(delay), Scheduler.Default)
        .Subscribe();

    for (var i = 0; i < count; ++i)
    {
        source.AddOrUpdate(i);
        //wait 10 times longer then the expiration time for the item
        await Task.Delay(TimeSpan.FromMilliseconds(10*delay));
        if(source.Count>0)
            Console.WriteLine($"Missed: {i} Count: {source.Count}");
    }
    
    source.Dispose();
}

Reproduction repository

https://github.com/reactivemarbles/DynamicData/

Expected behavior

There should be no output

Screenshots 🖼️

Sample output:
Missed: 2 Count: 1
Missed: 6 Count: 1
Missed: 7 Count: 1

This proves that adding new item will expire the previous and sometimes! the new one.

IDE

No response

Operating system

Happened on Mac and Linux container

Version

No response

Device

No response

DynamicData Version

7.14.2

Additional information ℹ️

No response

[JakenVeina]

It looks like a more direct workaround would be to supply the pollingInterval argument to .ExpireAfter(). That triggers single recurring background action to check all items for expiration, rather than scheduling a unique removal action per item.

DynamicData/src/DynamicData/List/Internal/ExpireAfter.cs

Line 73 in 0292f19

if (_pollingInterval.HasValue)

As for a fix, both mechanisms use the same RemovalAction() local method to perform removal. That should probably be refactored to perform removal by key instead, for non-polling expiration. That would mean that removal might occur slightly earlier than the specified expireAfter interval, due to inaccuracies in the scheduler, but that beats items getting orphaned completely.

[JakenVeina]

Actually, ignore me. You specifically referenced the SourceCache version of the operator, and I went and dug into the SourceList version.

I'm gonna go ahead and take the same approach here as I did for .ToObservableChangeSet(), I.E. beef up the testing and benchmarking on the existing versions, then rebuild the operators from scratch, based on that. Especially, considering time-based expiration is fresh on my mind after that effort.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.