Merge pull request #1329 from microsoft/fix1324

AArnott · web-flow · commit 1beb6ff56ff9 · 2025-01-17T16:23:49.000-07:00
Allow deletion of the metadata files while the compiler process is running
diff --git a/src/Microsoft.Windows.CsWin32/Generator.cs b/src/Microsoft.Windows.CsWin32/Generator.cs
@@ -35,7 +35,7 @@ public partial class Generator : IGenerator, IDisposable
     private readonly StructDeclarationSyntax variableLengthInlineArrayStruct2;
 
     private readonly Dictionary<string, IReadOnlyList<ISymbol>> findTypeSymbolIfAlreadyAvailableCache = new(StringComparer.Ordinal);
-    private readonly Rental<MetadataReader> metadataReader;
+    private readonly MetadataFile.Rental metadataReader;
     private readonly GeneratorOptions options;
     private readonly CSharpCompilation? compilation;
     private readonly CSharpParseOptions? parseOptions;
@@ -85,9 +85,11 @@ public Generator(string metadataLibraryPath, Docs? docs, GeneratorOptions option
             throw new ArgumentNullException(nameof(options));
         }
 
-        this.MetadataIndex = MetadataIndex.Get(metadataLibraryPath, compilation?.Options.Platform);
+        MetadataFile metadataFile = MetadataCache.Default.GetMetadataFile(metadataLibraryPath);
+        this.MetadataIndex = metadataFile.GetMetadataIndex(compilation?.Options.Platform);
+        this.metadataReader = metadataFile.GetMetadataReader();
+
         this.ApiDocs = docs;
-        this.metadataReader = MetadataIndex.GetMetadataReader(metadataLibraryPath);
 
         this.options = options;
         this.options.Validate();
diff --git a/src/Microsoft.Windows.CsWin32/MetadataCache.cs b/src/Microsoft.Windows.CsWin32/MetadataCache.cs
@@ -0,0 +1,41 @@
+﻿// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace Microsoft.Windows.CsWin32;
+
+internal class MetadataCache
+{
+    internal static readonly MetadataCache Default = new();
+
+    private readonly Dictionary<string, MetadataFile> metadataFiles = new(StringComparer.OrdinalIgnoreCase);
+
+    /// <summary>
+    /// Gets a file accessor for the given path that supports many concurrent readers.
+    /// </summary>
+    /// <param name="path">The path to the .winmd file.</param>
+    /// <returns>The file accessor.</returns>
+    internal MetadataFile GetMetadataFile(string path)
+    {
+        lock (this.metadataFiles)
+        {
+            MetadataFile? metadataFile;
+            DateTime lastWriteTimeUtc = File.GetLastWriteTimeUtc(path);
+            if (this.metadataFiles.TryGetValue(path, out metadataFile))
+            {
+                if (metadataFile.LastWriteTimeUtc == lastWriteTimeUtc)
+                {
+                    // We already have the file, and it is still current. Happy path.
+                    return metadataFile;
+                }
+
+                // Stale file. Evict from the cache.
+                this.metadataFiles.Remove(path);
+                metadataFile.Dispose();
+            }
+
+            // New or updated file. Re-open.
+            this.metadataFiles.Add(path, metadataFile = new MetadataFile(path));
+            return metadataFile;
+        }
+    }
+}
diff --git a/src/Microsoft.Windows.CsWin32/MetadataFile.cs b/src/Microsoft.Windows.CsWin32/MetadataFile.cs
@@ -0,0 +1,147 @@
+﻿// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.IO.MemoryMappedFiles;
+using System.Reflection.PortableExecutable;
+
+namespace Microsoft.Windows.CsWin32;
+
+[DebuggerDisplay($"{{{nameof(DebuggerDisplay)},nq}}")]
+internal class MetadataFile : IDisposable
+{
+    private readonly object syncObject = new();
+    private readonly Stack<(PEReader PEReader, MetadataReader MDReader)> peReaders = new();
+    private readonly Dictionary<Platform?, MetadataIndex> indexes = new();
+    private int readersRentedOut;
+    private MemoryMappedFile file;
+    private bool obsolete;
+
+    internal MetadataFile(string path)
+    {
+        this.Path = path;
+        this.LastWriteTimeUtc = File.GetLastWriteTimeUtc(path);
+
+        // When using FileShare.Delete, the OS will allow the file to be deleted, but it does not disrupt
+        // our ability to read the file while our handle is open.
+        // The file may be recreated on disk as well, and we'll keep reading the original file until we close that handle.
+        // We may also open the new file while holding the old handle,
+        // at which point we have handles open to both versions of the file concurrently.
+        FileStream metadataStream = new(path, FileMode.Open, FileAccess.Read, FileShare.Read | FileShare.Delete);
+        this.file = MemoryMappedFile.CreateFromFile(metadataStream, mapName: null, capacity: 0, MemoryMappedFileAccess.Read, HandleInheritability.None, leaveOpen: false);
+    }
+
+    internal string Path { get; }
+
+    internal DateTime LastWriteTimeUtc { get; }
+
+    private string DebuggerDisplay => $"\"{this.Path}\" ({this.LastWriteTimeUtc})";
+
+    /// <summary>
+    /// Prepares to close the file handle and release resources as soon as all rentals have been returned.
+    /// </summary>
+    public void Dispose()
+    {
+        lock (this.syncObject)
+        {
+            this.obsolete = true;
+
+            // Drain our cache of readers (the ones that aren't currently being used).
+            while (this.peReaders.Count > 0)
+            {
+                this.peReaders.Pop().PEReader.Dispose();
+            }
+
+            // Close the file if we have no readers rented out.
+            if (this.readersRentedOut == 0)
+            {
+                this.file.Dispose();
+            }
+        }
+    }
+
+    internal Rental GetMetadataReader()
+    {
+        lock (this.syncObject)
+        {
+            if (this.obsolete)
+            {
+                throw new InvalidOperationException("This file was deleted and should no longer be used.");
+            }
+
+            PEReader peReader;
+            MetadataReader metadataReader;
+            if (this.peReaders.Count > 0)
+            {
+                (peReader, metadataReader) = this.peReaders.Pop();
+            }
+            else
+            {
+                peReader = new(this.file.CreateViewStream(offset: 0, size: 0, MemoryMappedFileAccess.Read));
+                metadataReader = peReader.GetMetadataReader();
+            }
+
+            this.readersRentedOut++;
+            return new Rental(peReader, metadataReader, this);
+        }
+    }
+
+    internal MetadataIndex GetMetadataIndex(Platform? platform)
+    {
+        lock (this.syncObject)
+        {
+            if (!this.indexes.TryGetValue(platform, out MetadataIndex? index))
+            {
+                this.indexes.Add(platform, index = new MetadataIndex(this, platform));
+            }
+
+            return index;
+        }
+    }
+
+    private void ReturnReader(PEReader peReader, MetadataReader mdReader)
+    {
+        lock (this.syncObject)
+        {
+            this.readersRentedOut--;
+            Debug.Assert(this.readersRentedOut >= 0, "Some reader was returned more than once.");
+
+            if (this.obsolete)
+            {
+                // This file has been marked as stale, so we don't want to recycle the reader.
+                peReader.Dispose();
+
+                // If this was the last rental to be returned, we can close the file.
+                if (this.readersRentedOut == 0)
+                {
+                    this.file.Dispose();
+                }
+            }
+            else
+            {
+                // Store this in the cache for reuse later.
+                this.peReaders.Push((peReader, mdReader));
+            }
+        }
+    }
+
+    internal class Rental : IDisposable
+    {
+        private (PEReader PEReader, MetadataReader MDReader, MetadataFile File)? state;
+
+        internal Rental(PEReader peReader, MetadataReader mdReader, MetadataFile file)
+        {
+            this.state = (peReader, mdReader, file);
+        }
+
+        internal MetadataReader Value => this.state?.MDReader ?? throw new ObjectDisposedException(typeof(Rental).FullName);
+
+        public void Dispose()
+        {
+            if (this.state is (PEReader peReader, MetadataReader mdReader, MetadataFile file))
+            {
+                file.ReturnReader(peReader, mdReader);
+                this.state = null;
+            }
+        }
+    }
+}
diff --git a/src/Microsoft.Windows.CsWin32/MetadataIndex.cs b/src/Microsoft.Windows.CsWin32/MetadataIndex.cs
@@ -3,8 +3,6 @@
 
 using System.Collections.Concurrent;
 using System.Collections.ObjectModel;
-using System.IO.MemoryMappedFiles;
-using System.Reflection.PortableExecutable;
 
 namespace Microsoft.Windows.CsWin32;
 
@@ -25,19 +23,7 @@ internal class MetadataIndex
 {
     private static readonly int MaxPooledObjectCount = Math.Max(Environment.ProcessorCount, 4);
 
-    private static readonly Action<MetadataReader, object?> ReaderRecycleDelegate = Recycle;
-
-    private static readonly Dictionary<CacheKey, MetadataIndex> Cache = new();
-
-    /// <summary>
-    /// A cache of metadata files read.
-    /// All access to this should be within a <see cref="Cache"/> lock.
-    /// </summary>
-    private static readonly Dictionary<string, MemoryMappedFile> MetadataFiles = new(StringComparer.OrdinalIgnoreCase);
-
-    private static readonly ConcurrentDictionary<string, ConcurrentBag<(PEReader, MetadataReader)>> PooledPEReaders = new(StringComparer.OrdinalIgnoreCase);
-
-    private readonly string metadataPath;
+    private readonly MetadataFile metadataFile;
 
     private readonly Platform? platform;
 
@@ -72,14 +58,14 @@ internal class MetadataIndex
     /// <summary>
     /// Initializes a new instance of the <see cref="MetadataIndex"/> class.
     /// </summary>
-    /// <param name="metadataPath">The path to the metadata that this index will represent.</param>
+    /// <param name="metadataFile">The metadata file that this index will represent.</param>
     /// <param name="platform">The platform filter to apply when reading the metadata.</param>
-    private MetadataIndex(string metadataPath, Platform? platform)
+    internal MetadataIndex(MetadataFile metadataFile, Platform? platform)
     {
-        this.metadataPath = metadataPath;
+        this.metadataFile = metadataFile;
         this.platform = platform;
 
-        using Rental<MetadataReader> mrRental = GetMetadataReader(metadataPath);
+        using MetadataFile.Rental mrRental = metadataFile.GetMetadataReader();
         MetadataReader mr = mrRental.Value;
         this.MetadataName = Path.GetFileNameWithoutExtension(mr.GetString(mr.GetAssemblyDefinition().Name));
 
@@ -246,50 +232,7 @@ void PopulateNamespace(NamespaceDefinition ns, string? parentNamespace)
 
     internal string CommonNamespaceDot { get; }
 
-    private string DebuggerDisplay => $"{this.metadataPath} ({this.platform})";
-
-    internal static MetadataIndex Get(string metadataPath, Platform? platform)
-    {
-        metadataPath = Path.GetFullPath(metadataPath);
-        CacheKey key = new(metadataPath, platform);
-        lock (Cache)
-        {
-            if (!Cache.TryGetValue(key, out MetadataIndex index))
-            {
-                Cache.Add(key, index = new MetadataIndex(metadataPath, platform));
-            }
-
-            return index;
-        }
-    }
-
-    internal static Rental<MetadataReader> GetMetadataReader(string metadataPath)
-    {
-        if (PooledPEReaders.TryGetValue(metadataPath, out ConcurrentBag<(PEReader, MetadataReader)>? pool) && pool.TryTake(out (PEReader, MetadataReader) readers))
-        {
-            return new(readers.Item2, ReaderRecycleDelegate, (readers.Item1, metadataPath));
-        }
-
-        PEReader peReader = new PEReader(CreateFileView(metadataPath));
-        return new(peReader.GetMetadataReader(), ReaderRecycleDelegate, (peReader, metadataPath));
-    }
-
-    internal static MemoryMappedViewStream CreateFileView(string metadataPath)
-    {
-        lock (Cache)
-        {
-            // We use a memory mapped file so that many threads can perform random access on it concurrently,
-            // only mapping the file into memory once.
-            if (!MetadataFiles.TryGetValue(metadataPath, out MemoryMappedFile? file))
-            {
-                var metadataStream = new FileStream(metadataPath, FileMode.Open, FileAccess.Read, FileShare.Read);
-                file = MemoryMappedFile.CreateFromFile(metadataStream, mapName: null, capacity: 0, MemoryMappedFileAccess.Read, HandleInheritability.None, leaveOpen: false);
-                MetadataFiles.Add(metadataPath, file);
-            }
-
-            return file.CreateViewStream(offset: 0, size: 0, MemoryMappedFileAccess.Read);
-        }
-    }
+    private string DebuggerDisplay => $"{this.metadataFile.Path} ({this.platform})";
 
     /// <summary>
     /// Attempts to translate a <see cref="TypeReferenceHandle"/> to a <see cref="TypeDefinitionHandle"/>.
@@ -423,21 +366,6 @@ internal bool TryGetEnumName(MetadataReader reader, string enumValueName, [NotNu
         return false;
     }
 
-    private static void Recycle(MetadataReader metadataReader, object? state)
-    {
-        (PEReader peReader, string metadataPath) = ((PEReader, string))state!;
-        ConcurrentBag<(PEReader, MetadataReader)> pool = PooledPEReaders.GetOrAdd(metadataPath, _ => new());
-        if (pool.Count < MaxPooledObjectCount)
-        {
-            pool.Add((peReader, metadataReader));
-        }
-        else
-        {
-            // The pool is full. Dispose of this rather than recycle it.
-            peReader.Dispose();
-        }
-    }
-
     private static string CommonPrefix(IReadOnlyList<string> ss)
     {
         if (ss.Count == 0)
@@ -497,33 +425,4 @@ private static string CommonPrefix(IReadOnlyList<string> ss)
         // Return null if the value was determined to be missing.
         return this.enumTypeReference.HasValue && !this.enumTypeReference.Value.IsNil ? this.enumTypeReference.Value : null;
     }
-
-    [DebuggerDisplay("{" + nameof(DebuggerDisplay) + ",nq}")]
-    private struct CacheKey : IEquatable<CacheKey>
-    {
-        internal CacheKey(string metadataPath, Platform? platform)
-        {
-            this.MetadataPath = metadataPath;
-            this.Platform = platform;
-        }
-
-        internal string MetadataPath { get; }
-
-        internal Platform? Platform { get; }
-
-        private string DebuggerDisplay => $"{this.MetadataPath} ({this.Platform})";
-
-        public override bool Equals(object obj) => obj is CacheKey other && this.Equals(other);
-
-        public bool Equals(CacheKey other)
-        {
-            return this.Platform == other.Platform
-                && string.Equals(this.MetadataPath, other.MetadataPath, StringComparison.OrdinalIgnoreCase);
-        }
-
-        public override int GetHashCode()
-        {
-            return StringComparer.OrdinalIgnoreCase.GetHashCode(this.MetadataPath) + (this.Platform.HasValue ? (int)this.Platform.Value : 0);
-        }
-    }
 }
diff --git a/src/Microsoft.Windows.CsWin32/SourceGenerator.cs b/src/Microsoft.Windows.CsWin32/SourceGenerator.cs
diff --git a/test/Microsoft.Windows.CsWin32.Tests/GeneratorTestBase.cs b/test/Microsoft.Windows.CsWin32.Tests/GeneratorTestBase.cs
