[Windows] Illustration of fix in FileSystemWatcher to prevent SOH fragmentation from memory leak of pinned 8K byte[] buffers

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs

@@ -49,7 +49,14 @@ private void StartRaisingEvents()
                 // Start ignoring all events that were initiated before this, and
                 // allocate the buffer to be pinned and used for the duration of the operation
                 int session = Interlocked.Increment(ref _currentSession);
-                byte[] buffer = AllocateBuffer();
+
+                /*
+                 * Note: This is just an illustrative PR, we have more changes to make, including:
+                 *       1. Checking if we need to use GC.AllocateArray<byte> on Linux as well
+                 *       2. Removing the need to pin the buffer using GCHandle later on when provisioning / Packing the ThreadPoolBoundHandleOverlapped,
+                 *          which is done when allocating a new PreAllocatedOverlapped below.
+                 */
+                byte[] buffer = AllocateBuffer(pinned: true);
 
                 // Store all state, including a preallocated overlapped, into the state object that'll be
                 // passed from iteration to iteration during the lifetime of the operation.  The buffer will be pinned
@@ -60,11 +67,69 @@ private void StartRaisingEvents()
                     state.PreAllocatedOverlapped = new PreAllocatedOverlapped((errorCode, numBytes, overlappedPointer) =>
                     {
                         AsyncReadState state = (AsyncReadState)ThreadPoolBoundHandle.GetNativeOverlappedState(overlappedPointer)!;
+
+                        /*
+                         * Note: Freeing the NativeOverlapped via ThreadPoolBoundHandle will only call AsyncState.PreAllocatedOverlapped.Release(),
+                         *       and NOT AsyncState.PreAllocatedOverlapped.Dispose() which is what unpins and frees up the pinned 8K byte[] buffer,
+                         *       as well as the ThreadPoolBoundHandleOverlapped which is also held by a strong GCHandle and prevented from GC.
+                         *       In our process dump which has a leak of the pinned 8K byte[] buffer, we can see evidence that AsyncState.PreAllocatedOverlapped.Release()
+                         *       was performed, namely for each leaked pinned buffer we see that:
+                         *       1) AsyncState.PreAllocatedOverlapped._lifetime count = 0
+                         *       2) ThreadPoolBoundHandleOverlapped._boundHandle = NULL
+                         *       3) ThreadPoolBoundHandleOverlapped._completed = FALSE
+                         *       4) ThreadPoolBoundHandleOverlapped._nativeOverlapped is zeroed out (default for the struct)
+                         */
                         state.ThreadPoolBinding.FreeNativeOverlapped(overlappedPointer);
+
+                        /*
+                         * When we reach here, app that's using this FileSystemWatcher instance may have given it up for GC, having no use for it anywmore,
+                         * since the last time Monitor() call queued an async overlapped ReadDirectoryChangesW operation.
+                         * There are 2 sub-cases:
+                         * 1) When app has disposed the FileSystemWatcher before releasing the last reference
+                         *    Disposal of FileSystemWatcher will dispose the underlying directory SafeFileHandle and kick off a race between:
+                         *    a) The IOCP thread executing the FileSystemWatcher directory changes callback, which will come in with
+                         *       errorCode = ERROR_OPERATION_ABORTED, and simultaneously,
+                         *    b) The dedicated Server GC threads trying to collect the FileSystemWatcher after app gives up its last reference.
+                         *    If a) wins the race, everything is fine :)
+                         *          WHY? The AsycState's WeakRef<FileSystemwatcher> in this callback will be intact, so watcher.ReadDirectoryChangesCallback
+                         *          will be called, it will find out that the directory handle is invalid now, and it will kick off the next Monitor() call,
+                         *          which will in turn find out that the directory handle is invalid now and proceed to properly call PreAllocatedOverlapped.Dispose()
+                         *          to unpin and free up the pinned 8K byte[] buffer.
+                         *    However, if b) wins the race, !!  WE HAVE A HEAP LEAK !!
+                         *          WHY? The AsycState's WeakRef<FileSystemwatcher> in this callback will be nulled out as soon as watcher is queued for finalization,
+                         *          especially since its a *SHORT* WeakRef, and the callback will not be able to find the watcher anymore.
+                         *          so watcher.ReadDirectoryChangesCallback is not at all called below, so there is no chance of disposing the PreAllocatedOverlapped
+                         *          and freeing up the pinned 8K byte[] buffer and we have leaked it for eternity.
+                         * 2) When app has not disposed the FileSystemWatcher before releasing the last reference
+                         *    This case makes the heap leak fully deterministic, and not probabilistic, because the watcher is disposed from Finalizer thread,
+                         *    which will dispose the underlying directory SafeFileHandle and schedule the directory changes callback (errorCode = ERROR_OPERATION_ABORTED)
+                         *    on IOCP thread, but by that time, the AsycState's WeakRef<FileSystemwatcher> in the callback will *ALWAYS* be nulled out,
+                         *    since its a *SHORT* WeakRef.
+                         *    So watcher.ReadDirectoryChangesCallback is not at all called, so there is no chance of disposing the PreAllocatedOverlapped
+                         *    and freeing up the pinned 8K byte[] buffer and we have leaked it for eternity.
+                         *
+                         * Note: The memory leak has a higher probability of occurring in Server GC, since it pauses all threads, including the IOCP threads,
+                         *       executing this callbacks, so this enhances the ability of GC to win the above race and cause a leak.
+                         */
+
                         if (state.WeakWatcher.TryGetTarget(out FileSystemWatcher? watcher))
                         {
                             watcher.ReadDirectoryChangesCallback(errorCode, numBytes, state);
                         }
+                        else
+                        {
+                            /*
+                             * The app has no use for the FileSystemWatcher anymore, and it has been GCed. It may not be fully finalized yet,
+                             * so we cannot make assumptions about the state of the directory handle itself.
+                             * But we ought to cleanup the PreAllocatedOverlapped and the ThreadPoolBoundHandle properly, else we risk
+                             * leaking the pinned 8K byte[] buffer and the ThreadPoolBoundHandleOverlapped both prevented from GC by GCHandles.
+                             * The pinned 8K byte[] buffer is especially important, as it is pinned on SOH (not even on dedicated POH) so even
+                             * though its only 8K, too many of those can cause heavy heap fragmentation and prevent compaction of SOH, leading to
+                             * large memory usage (with mostly free space), poor allocation and GC performance, etc.
+                             */
+                            state.PreAllocatedOverlapped?.Dispose();
+                            state.ThreadPoolBinding?.Dispose();
+                        }
                     }, state, buffer);
                 }
             }

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs

@@ -225,11 +225,15 @@
 
         /// <summary>Allocates a buffer of the requested internal buffer size.</summary>
         /// <returns>The allocated buffer.</returns>
-        private byte[] AllocateBuffer()
+        private byte[] AllocateBuffer(bool pinned = false)
         {
             try
             {
-                return new byte[_internalBufferSize];
+                /*
+                 * Note: Although fixing the memory leak will arrest the probability of too many of these pinned 8K byte[] buffers
+                 *       fragmenting the SOH, we ought to take advantage of the POH to avoid fragmentation in the first place, for cleanliness sake.
+                 */
+                return pinned ? GC.AllocateArray<byte>(_internalBufferSize, pinned: true) : new byte[_internalBufferSize];
             }
             catch (OutOfMemoryException)
             {

src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherHeapLeak/FileSystemWatcherHeapLeak.csproj

@@ -0,0 +1,11 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <TargetFramework>net8.0</TargetFramework>
+    <ImplicitUsings>enable</ImplicitUsings>
+    <Nullable>enable</Nullable>
+    <ServerGarbageCollection>true</ServerGarbageCollection>
+  </PropertyGroup>
+
+</Project>

src/libraries/System.IO.FileSystem.Watcher/tests/Utility/FileSystemWatcherHeapLeak/Program.cs

@@ -0,0 +1,214 @@
+using System;
+using System.Runtime;
+
+namespace FileSystemWatcherHeapLeak
+{
+    internal class Program
+    {
+        static void Main(string[] args)
+        {
+            /*
+             * Set both min and max IOCP threads to 1 to serialize directory change callbacks
+             * and give GC a higher chance of winning the race after disposing the FileSystemWatcher.
+             */
+            ThreadPool.GetMinThreads(out int minWorker, out _);
+            ThreadPool.GetMaxThreads(out int maxWorker, out _);
+            ThreadPool.SetMinThreads(minWorker, 1);
+            ThreadPool.SetMaxThreads(maxWorker, 1);
+
+            Console.WriteLine($"Starting {nameof(FileSystemWatcher)} heap leak test...");
+            Console.WriteLine("Press Ctrl+C to stop the test.");
+
+            int disposeProbability = 100;
+            while (true)
+            {
+                Console.Write("Enter probability (0-100) to dispose FileSystemWatcher after each run: ");
+                var input = Console.ReadLine();
+                if (int.TryParse(input, out disposeProbability) &&
+                    disposeProbability >= 0 && disposeProbability <= 100)
+                {
+                    break;
+                }
+                else
+                {
+                    Console.WriteLine("Please enter a valid integer between 0 and 100.");
+                }
+            }
+
+            int maxGeneration = 2;
+            while (true)
+            {
+                Console.Write("Enter max GC generation to collect (0, 1, or 2): ");
+                var input = Console.ReadLine();
+                if (int.TryParse(input, out maxGeneration) && maxGeneration >= 0 && maxGeneration <= GC.MaxGeneration)
+                {
+                    break;
+                }
+                else
+                {
+                    Console.WriteLine($"Please enter a valid integer between 0 and {GC.MaxGeneration}.");
+                }
+            }
+
+            bool compacting = false;
+            while (true)
+            {
+                Console.Write("Enable compacting GC? (y/n): ");
+                var input = Console.ReadLine();
+                if (string.Equals(input, "y", StringComparison.OrdinalIgnoreCase))
+                {
+                    compacting = true;
+                    break;
+                }
+                else if (string.Equals(input, "n", StringComparison.OrdinalIgnoreCase))
+                {
+                    compacting = false;
+                    break;
+                }
+                else
+                {
+                    Console.WriteLine("Please enter 'y' or 'n'.");
+                }
+            }
+
+            Console.WriteLine($"Spawning {minWorker} parallel test loops (one per minWorkerThread).");
+
+            var tasks = new Task[minWorker];
+
+            int i = 0;
+            for (i = 0; i < minWorker - 1; i++)
+            {
+                int taskId = i; // for closure capture
+
+                tasks[i] = Task.Run(() =>
+                {
+                    var rng = new Random(Environment.TickCount ^ taskId);
+
+                    while (true)
+                    {
+                        try
+                        {
+                            bool wasDisposed = false;
+                            FileSystemWatcher watcher = null;
+
+                            try
+                            {
+                                watcher = new FileSystemWatcher(@"C:\Temp");
+                                watcher.EnableRaisingEvents = true;
+
+                                Thread.Sleep(rng.Next(5, 10)); // Keep the watcher alive for a short while
+                            }
+                            finally
+                            {
+                                wasDisposed = rng.Next(0, 100) < disposeProbability;
+
+                                if (wasDisposed)
+                                {
+                                    watcher?.Dispose();
+                                }
+
+                                watcher = null;
+                            }
+
+                            /*
+                             * When we reach here, there are 2 cases:
+                             * 1) When we have disposed the FileSystemWatcher
+                             *    Disposal of FileSystemWatcher will dispose the underlying directory SafeFileHandle and kick off a race between:
+                             *    a) The IOCP thread executing the FileSystemWatcher directory changes callback, which will come in with
+                             *       errorCode = ERROR_OPERATION_ABORTED, and simultaneously,
+                             *    b) The dedicated Server GC threads trying to collect the heap triggered below.
+                             *    If a) wins the race, everything is fine :)
+                             *          WHY? The AsycState's WeakRef<FileSystemwatcher> in the callback will be intact, so watcher.ReadDirectoryChangesCallback
+                             *          will be called, it will find out that the directory handle is invalid now, and it will kick off the next Monitor() call,
+                             *          which will in turn find out that the directory handle is invalid now and proceed to properly call PreAllocatedOverlapped.Dispose()
+                             *          to unpin and free up the pinned 8K byte[] buffer.
+                             *    However, if b) wins the race, !!  WE HAVE A HEAP LEAK !!
+                             *          WHY? The AsycState's WeakRef<FileSystemwatcher> in the callback will be nulled out as soon as watcher is queued for finalization,
+                             *          especially since its a *SHORT* WeakRef, and the callback will not be able to find the watcher anymore.
+                             *          so watcher.ReadDirectoryChangesCallback is not at all called, so there is no chance of disposing the PreAllocatedOverlapped
+                             *          and freeing up the pinned 8K byte[] buffer and we have leaked it for eternity.
+                             * 2) When we have not disposed the FileSystemWatcher
+                             *    This case makes the heap leak fully deterministic, and not probabilistic, because the watcher is disposed from Finalizer thread,
+                             *    which will dispose the underlying directory SafeFileHandle and schedule the directory changes callback on IOCP thread, but by that time,
+                             *    the AsycState's WeakRef<FileSystemwatcher> in the callback will *ALWAYS* be nulled out, since its a *SHORT* WeakRef.
+                             *    so watcher.ReadDirectoryChangesCallback is not at all called, so there is no chance of disposing the PreAllocatedOverlapped
+                             *    and freeing up the pinned 8K byte[] buffer and we have leaked it for eternity.
+                             * 
+                             * Note: We have already enabled Server GC mode in the project file, so as to pause all threads
+                             *       including the IOCP threads executing the FileSystemWatcher directory changes callbacks.
+                             *       This will enhance the ability of GC to win the above race after disposing the watcher.
+                             */
+
+                            if (compacting)
+                            {
+                                GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.CompactOnce;
+                            }
+                            else
+                            {
+                                GCSettings.LargeObjectHeapCompactionMode = GCLargeObjectHeapCompactionMode.Default;
+                            }
+
+                            GC.Collect(
+                                maxGeneration,
+                                GCCollectionMode.Forced | GCCollectionMode.Aggressive,
+                                blocking: true,
+                                compacting: compacting);
+
+                            var gcInfo = GC.GetGCMemoryInfo();
+
+                            Console.WriteLine(
+                                $"[Thread {taskId}] Watcher was {(wasDisposed ? string.Empty : "not ")}disposed, " +
+                                $"GC Gen: {maxGeneration}, Compacting: {compacting}, " +
+                                $"Commited Bytes = {gcInfo.TotalCommittedBytes}, " +
+                                $"Heap Size Bytes = {gcInfo.HeapSizeBytes}, " +
+                                $"Fragmented Bytes = {gcInfo.FragmentedBytes}, " +
+                                $"Fragmentation % = {100.0 * (double)gcInfo.FragmentedBytes / (double)gcInfo.TotalCommittedBytes}, " +
+                                $"Pinned Object Count = {gcInfo.PinnedObjectsCount}.");
+                        }
+                        catch (Exception ex)
+                        {
+                            Console.WriteLine($"[Thread {taskId}] Exception encountered but continuing to next iteration. Exception: {ex.Message}");
+                        }
+                    }
+                });
+            }
+
+            /*
+             * The last task is reserved for a thread that will allocate large amount of memory
+             * on SOH in parallel with other threads, and give a random chance for those allocs
+             * to survive GC generations, thus creating pockets of free memory between the byte[]
+             * instances that get leaked by the activity of other threads, inducing fragmentation.
+             * This thread is designed to resemble the SOH allocations that are made in our server app
+             * to conduct its business, all of which are mostly ephemeral and short-lived.
+             */
+            tasks[i] = Task.Run(() =>
+            {
+                var rng = new Random(Environment.TickCount ^ i);
+
+                const int chunkSize = 50 * 1024; // 50 KB (making sure we allocate on SOH to that it gets fragmented)
+                const int minTotal = 500 * 1024; // 500 KB
+                const int maxTotal = 1024 * 1024;// 1 MB
+
+                while (true)
+                {
+                    int totalToAllocate = rng.Next(minTotal, maxTotal + 1);
+                    int allocated = 0;
+                    var allocations = new List<string>();
+
+                    while (allocated < totalToAllocate)
+                    {
+                        int charCount = chunkSize / 2;
+                        string s = new string('A', charCount);
+                        allocations.Add(s);
+                        allocated += chunkSize;
+                    }
+
+                    Thread.Sleep(rng.Next(5, 50)); // Hold on to allocs for a while, giving a random chance for it to survive GC generations
+                    allocations.Clear();           // Then release them and other threads will force GC to collect them.
+                }
+            });
+
+            Task.WaitAll(tasks); // This will block forever unless the process is killed
+        }
+    }
+}