look into CI failure; timing

Author: mgravell

src/Caching/Hybrid/src/Internal/DefaultHybridCache.Debug.cs

@@ -39,4 +39,32 @@ internal void DebugIncrementOutstandingBuffers()
         Interlocked.Increment(ref _outstandingBufferCount);
     }
 #endif
+
+    partial class MutableCacheItem<T>
+    {
+        partial void DebugDecrementOutstandingBuffers();
+        partial void DebugTrackBufferCore(DefaultHybridCache cache);
+
+        [Conditional("DEBUG")]
+        internal void DebugTrackBuffer(DefaultHybridCache cache) => DebugTrackBufferCore(cache);
+
+#if DEBUG
+        private DefaultHybridCache? _cache; // for buffer-tracking - only enabled in DEBUG
+        partial void DebugDecrementOutstandingBuffers()
+        {
+            if (_buffer.ReturnToPool)
+            {
+                _cache?.DebugDecrementOutstandingBuffers();
+            }
+        }
+        partial void DebugTrackBufferCore(DefaultHybridCache cache)
+        {
+            _cache = cache;
+            if (_buffer.ReturnToPool)
+            {
+                _cache?.DebugIncrementOutstandingBuffers();
+            }
+        }
+#endif
+    }
 }

src/Caching/Hybrid/src/Internal/DefaultHybridCache.MutableCacheItem.cs

@@ -9,22 +9,11 @@ namespace Microsoft.Extensions.Caching.Hybrid.Internal;
 
 partial class DefaultHybridCache
 {
-    private sealed class MutableCacheItem<T> : CacheItem<T> // used to hold types that require defensive copies
+    private sealed partial class MutableCacheItem<T> : CacheItem<T> // used to hold types that require defensive copies
     {
         private readonly IHybridCacheSerializer<T> _serializer;
         private readonly BufferChunk _buffer;
         private int _refCount = 1; // buffer released when this becomes zero
-#if DEBUG
-        private DefaultHybridCache? _cache; // for buffer-tracking - only enabled in DEBUG
-        internal void DebugTrackBuffer(DefaultHybridCache cache)
-        {
-            _cache = cache;
-            if (_buffer.ReturnToPool)
-            {
-                _cache.DebugIncrementOutstandingBuffers();
-            }
-        }
-#endif
 
         public MutableCacheItem(ref BufferChunk buffer, IHybridCacheSerializer<T> serializer)
         {
@@ -43,19 +32,14 @@ public MutableCacheItem(T value, IHybridCacheSerializer<T> serializer, int maxLe
             writer.Dispose(); // no buffers left (we just detached them), but just in case of other logic
         }
 
-        public override bool NeedsEvictionCallback => true;
+        public override bool NeedsEvictionCallback => _buffer.ReturnToPool;
 
         public override void Release()
         {
             var newCount = Interlocked.Decrement(ref _refCount);
             if (newCount == 0)
             {
-#if DEBUG // avoid even the property touch if not in debug
-                if (_buffer.ReturnToPool)
-                {
-                    _cache?.DebugDecrementOutstandingBuffers();
-                }
-#endif
+                DebugDecrementOutstandingBuffers();
                 _buffer.RecycleIfAppropriate();
             }
         }

src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs

@@ -180,9 +180,7 @@ private void SetResultAndRecycleIfAppropriate(ref BufferChunk value)
             {
                 // use the buffer directly as the backing in the cache-item; do *not* recycle now
                 var tmp = new MutableCacheItem<T>(ref value, serializer);
-#if DEBUG
-                tmp.DebugTrackBuffer(Cache);
-#endif
+                tmp.DebugTrackBuffer(Cache); // conditional: DEBUG
                 cacheItem = tmp;
             }
 
@@ -200,9 +198,7 @@ private CacheItem<T> SetResult(T value)
             else
             {
                 var tmp = new MutableCacheItem<T>(value, Cache.GetSerializer<T>(), MaximumPayloadBytes); // serialization happens here
-#if DEBUG
-                tmp.DebugTrackBuffer(Cache);
-#endif
+                tmp.DebugTrackBuffer(Cache); // conditional: DEBUG
                 cacheItem = tmp;
             }
             SetResult(cacheItem);

src/Caching/Hybrid/test/BufferReleaseTests.cs

@@ -1,26 +1,31 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Buffers;
 using System.Runtime.CompilerServices;
+using Microsoft.Extensions.Caching.Distributed;
 using Microsoft.Extensions.Caching.Hybrid.Internal;
+using Microsoft.Extensions.Caching.Memory;
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Options;
 using static Microsoft.Extensions.Caching.Hybrid.Internal.DefaultHybridCache;
 
 namespace Microsoft.Extensions.Caching.Hybrid.Tests;
 
 public class BufferReleaseTests // note that buffer ref-counting is only enabled for DEBUG builds; can only verify general behaviour without that
 {
-    static IDisposable GetDefaultCache(out DefaultHybridCache cache)
+    static IDisposable GetDefaultCache(out DefaultHybridCache cache, Action<ServiceCollection>? config = null)
     {
         var services = new ServiceCollection();
+        config?.Invoke(services);
         services.AddHybridCache();
         var provider = services.BuildServiceProvider();
         cache = Assert.IsType<DefaultHybridCache>(provider.GetRequiredService<HybridCache>());
         return provider;
     }
 
     [Fact]
-    public async Task BufferGetsReleased()
+    public async Task BufferGetsReleased_NoL2()
     {
         using var provider = GetDefaultCache(out var cache);
 #if DEBUG
@@ -39,27 +44,174 @@ public async Task BufferGetsReleased()
         Assert.True(cache.DebugTryGetCacheItem(key, out var cacheItem));
 
         // assert that we can reserve the buffer *now* (mostly to see that it behaves differently later)
+        Assert.True(cacheItem.NeedsEvictionCallback, "should be pooled memory");
         Assert.True(cacheItem.TryReserveBuffer(out _));
         cacheItem.Release(); // for the above reserve
 
-        var second = await cache.GetOrCreateAsync(key, _ => GetAsync(), new HybridCacheEntryOptions { Flags = HybridCacheEntryFlags.DisableUnderlyingData });
+        var second = await cache.GetOrCreateAsync(key, _ => GetAsync(), NoUnderlying);
         Assert.NotNull(second);
         Assert.NotSame(first, second);
 
         await cache.RemoveKeyAsync(key);
-        var third = await cache.GetOrCreateAsync(key, _ => GetAsync(), new HybridCacheEntryOptions { Flags = HybridCacheEntryFlags.DisableUnderlyingData });
+        var third = await cache.GetOrCreateAsync(key, _ => GetAsync(), NoUnderlying);
         Assert.Null(third);
 
-        await Task.Delay(500); // give it a moment
+        // give it a moment for the eviction callback to kick in
+        for (int i = 0; i < 10 && cacheItem.NeedsEvictionCallback; i++)
+        {
+            await Task.Delay(250);
+        }
 #if DEBUG
         Assert.Equal(0, cache.DebugGetOutstandingBuffers());
 #endif
         // assert that we can *no longer* reserve this buffer, because we've already recycled it
         Assert.False(cacheItem.TryReserveBuffer(out _));
+        Assert.False(cacheItem.NeedsEvictionCallback, "should be recycled now");
+        static ValueTask<Customer> GetAsync() => new(new Customer { Id = 42, Name = "Fred" });
+    }
+
+    private static readonly HybridCacheEntryOptions NoUnderlying = new HybridCacheEntryOptions { Flags = HybridCacheEntryFlags.DisableUnderlyingData };
+
+    class TestCache : MemoryDistributedCache, IBufferDistributedCache
+    {
+        public TestCache(IOptions<MemoryDistributedCacheOptions> options) : base(options) { }
 
+        void IBufferDistributedCache.Set(string key, ReadOnlySequence<byte> value, DistributedCacheEntryOptions options)
+            => Set(key, value.ToArray(), options); // efficiency not important for this
+
+        ValueTask IBufferDistributedCache.SetAsync(string key, ReadOnlySequence<byte> value, DistributedCacheEntryOptions options, CancellationToken token)
+            => new(SetAsync(key, value.ToArray(), options, token)); // efficiency not important for this
+
+        bool IBufferDistributedCache.TryGet(string key, IBufferWriter<byte> destination)
+            => Write(destination, Get(key));
+
+        async ValueTask<bool> IBufferDistributedCache.TryGetAsync(string key, IBufferWriter<byte> destination, CancellationToken token)
+            => Write(destination, await GetAsync(key, token));
+
+        static bool Write(IBufferWriter<byte> destination, byte[]? buffer)
+        {
+            if (buffer is null)
+            {
+                return false;
+            }
+            destination.Write(buffer);
+            return true;
+        }
+    }
+
+    [Fact]
+    public async Task BufferDoesNotNeedRelease_LegacyL2() // byte[] API; not pooled
+    {
+        using var provider = GetDefaultCache(out var cache,
+            services => services.AddSingleton<IDistributedCache, TestCache>());
+
+        cache.DebugRemoveFeatures(CacheFeatures.BackendBuffers);
+        // prep the backend with our data
+        var key = Me();
+        Assert.NotNull(cache.BackendCache);
+        var serializer = cache.GetSerializer<Customer>();
+        using (var writer = RecyclableArrayBufferWriter<byte>.Create(int.MaxValue))
+        {
+            serializer.Serialize(await GetAsync(), writer);
+            cache.BackendCache.Set(key, writer.ToArray());
+        }
+#if DEBUG
+        cache.DebugGetOutstandingBuffers(flush: true);
+        Assert.Equal(0, cache.DebugGetOutstandingBuffers());
+#endif
+        var first = await cache.GetOrCreateAsync(key, _ => GetAsync(), NoUnderlying); // we expect this to come from L2, hence NoUnderlying
+        Assert.NotNull(first);
+#if DEBUG
+        Assert.Equal(0, cache.DebugGetOutstandingBuffers());
+#endif
+        Assert.True(cache.DebugTryGetCacheItem(key, out var cacheItem));
+
+        // assert that we can reserve the buffer *now* (mostly to see that it behaves differently later)
+        Assert.False(cacheItem.NeedsEvictionCallback, "should NOT be pooled memory");
+        Assert.True(cacheItem.TryReserveBuffer(out _));
+        cacheItem.Release(); // for the above reserve
+
+        var second = await cache.GetOrCreateAsync(key, _ => GetAsync(), NoUnderlying);
+        Assert.NotNull(second);
+        Assert.NotSame(first, second);
+
+        await cache.RemoveKeyAsync(key);
+        var third = await cache.GetOrCreateAsync(key, _ => GetAsync(), NoUnderlying);
+        Assert.Null(third);
+        Assert.Null(await cache.BackendCache.GetAsync(key)); // should be gone from L2 too
+
+        // give it a moment for the eviction callback to kick in
+        for (int i = 0; i < 10 && cacheItem.NeedsEvictionCallback; i++)
+        {
+            await Task.Delay(250);
+        }
+#if DEBUG
+        Assert.Equal(0, cache.DebugGetOutstandingBuffers());
+#endif
+        // assert that we can *no longer* reserve this buffer, because we've already recycled it
+        Assert.True(cacheItem.TryReserveBuffer(out _)); // always readable
+        cacheItem.Release();
+
+        Assert.False(cacheItem.NeedsEvictionCallback, "should still not need recycling");
+        static ValueTask<Customer> GetAsync() => new(new Customer { Id = 42, Name = "Fred" });
+    }
+
+    [Fact]
+    public async Task BufferGetsReleased_BufferL2() // IBufferWriter<byte> API; pooled
+    {
+        using var provider = GetDefaultCache(out var cache,
+            services => services.AddSingleton<IDistributedCache, TestCache>());
+
+        // prep the backend with our data
+        var key = Me();
+        Assert.NotNull(cache.BackendCache);
+        var serializer = cache.GetSerializer<Customer>();
+        using (var writer = RecyclableArrayBufferWriter<byte>.Create(int.MaxValue))
+        {
+            serializer.Serialize(await GetAsync(), writer);
+            cache.BackendCache.Set(key, writer.ToArray());
+        }
+#if DEBUG
+        cache.DebugGetOutstandingBuffers(flush: true);
+        Assert.Equal(0, cache.DebugGetOutstandingBuffers());
+#endif
+        var first = await cache.GetOrCreateAsync(key, _ => GetAsync(), NoUnderlying); // we expect this to come from L2, hence NoUnderlying
+        Assert.NotNull(first);
+#if DEBUG
+        Assert.Equal(1, cache.DebugGetOutstandingBuffers());
+#endif
+        Assert.True(cache.DebugTryGetCacheItem(key, out var cacheItem));
+
+        // assert that we can reserve the buffer *now* (mostly to see that it behaves differently later)
+        Assert.True(cacheItem.NeedsEvictionCallback, "should be pooled memory");
+        Assert.True(cacheItem.TryReserveBuffer(out _));
+        cacheItem.Release(); // for the above reserve
+
+        var second = await cache.GetOrCreateAsync(key, _ => GetAsync(), NoUnderlying);
+        Assert.NotNull(second);
+        Assert.NotSame(first, second);
+
+        await cache.RemoveKeyAsync(key);
+        var third = await cache.GetOrCreateAsync(key, _ => GetAsync(), NoUnderlying);
+        Assert.Null(third);
+        Assert.Null(await cache.BackendCache.GetAsync(key)); // should be gone from L2 too
+
+        // give it a moment for the eviction callback to kick in
+        for (int i = 0; i < 10 && cacheItem.NeedsEvictionCallback; i++)
+        {
+            await Task.Delay(250);
+        }
+#if DEBUG
+        Assert.Equal(0, cache.DebugGetOutstandingBuffers());
+#endif
+        // assert that we can *no longer* reserve this buffer, because we've already recycled it
+        Assert.False(cacheItem.TryReserveBuffer(out _)); // released now
+
+        Assert.False(cacheItem.NeedsEvictionCallback, "should be recycled by now");
         static ValueTask<Customer> GetAsync() => new(new Customer { Id = 42, Name = "Fred" });
     }
 
+
     public class Customer
     {
         public int Id { get; set; }