Skip to content

Conversation

@bitfaster
Copy link
Owner

@bitfaster bitfaster commented Feb 1, 2025

When items are deleted, they remain in the internal queues until fully cycled out of cold. This is because the cycle/route methods do not inspect WasDeleted. Instead, purge them as items transition from queue to queue as part of cycle.

This change doesn't trigger double dispose/event due to the implementation of the Move method first attempting to remove the item from the dictionary:

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        private int Move(I item, ItemDestination where, ItemRemovedReason removedReason)
        {
            item.WasAccessed = false;

            switch (where)
            {
                case ItemDestination.Warm:
                    this.warmQueue.Enqueue(item);
                    return Interlocked.Increment(ref this.counter.warm);
                case ItemDestination.Cold:
                    this.coldQueue.Enqueue(item);
                    return Interlocked.Increment(ref this.counter.cold);
                case ItemDestination.Remove:

                    var kvp = new KeyValuePair<K, I>(item.Key, item);

                    // when the item is already removed, skip event/metric counting
                    if (this.dictionary.TryRemove(kvp))
                    {
                        OnRemove(item.Key, item, removedReason);
                    }
                    break;
            }

            return 0;
        }

Scenarios covered:

  • Item removed from hot during warmup
  • Item removed from hot after warmup
  • Item removed from warm during warmup
  • Item removed from warm after warmp
  • Item removed from cold after warmp
  • Item removed then call TrimExpired
  • Item removed then call Trim

Found two other bugs in Trim logic:

  • TrimExpired missed the lock to make it thread-safe
  • When trimmed item is in cold queue, we always trimmed an additional item from warm, which is incorrect.

@coveralls
Copy link

coveralls commented Feb 1, 2025

Coverage Status

coverage: 99.169% (-0.06%) from 99.229%
when pulling 2f68804 on users/alexpeck/remremoved
into 01805a5 on main.

@bitfaster bitfaster linked an issue Feb 2, 2025 that may be closed by this pull request
@bitfaster bitfaster changed the title Eagerly purged deleted items from internal LRU queues Eagerly purge deleted items from internal LRU queues Feb 2, 2025
@bitfaster bitfaster marked this pull request as ready for review February 3, 2025 01:14
@bitfaster bitfaster merged commit da00197 into main Feb 3, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ConcurrentTLru leaks memory in removed element's values

3 participants