From 6130ce9be813c84a156a0e74043e84622d163d88 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Fri, 17 Jan 2025 15:58:27 -0700 Subject: [PATCH] Allow deletion of the metadata files while the compiler process is running Fixes #1324 --- src/Microsoft.Windows.CsWin32/Generator.cs | 8 +- .../MetadataCache.cs | 41 +++++ src/Microsoft.Windows.CsWin32/MetadataFile.cs | 147 +++++++++++++++ .../MetadataIndex.cs | 113 +----------- .../SourceGenerator.cs | 173 +++++++++--------- .../GeneratorTestBase.cs | 6 +- 6 files changed, 287 insertions(+), 201 deletions(-) create mode 100644 src/Microsoft.Windows.CsWin32/MetadataCache.cs create mode 100644 src/Microsoft.Windows.CsWin32/MetadataFile.cs diff --git a/src/Microsoft.Windows.CsWin32/Generator.cs b/src/Microsoft.Windows.CsWin32/Generator.cs index afc9d7fa..f167af6f 100644 --- 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> findTypeSymbolIfAlreadyAvailableCache = new(StringComparer.Ordinal); - private readonly Rental 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 new file mode 100644 index 00000000..18073a13 --- /dev/null +++ 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 metadataFiles = new(StringComparer.OrdinalIgnoreCase); + + /// + /// Gets a file accessor for the given path that supports many concurrent readers. + /// + /// The path to the .winmd file. + /// The file accessor. + 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 new file mode 100644 index 00000000..3aee831b --- /dev/null +++ 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 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})"; + + /// + /// Prepares to close the file handle and release resources as soon as all rentals have been returned. + /// + 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 index 9d03f6b7..39db44c1 100644 --- 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 ReaderRecycleDelegate = Recycle; - - private static readonly Dictionary Cache = new(); - - /// - /// A cache of metadata files read. - /// All access to this should be within a lock. - /// - private static readonly Dictionary MetadataFiles = new(StringComparer.OrdinalIgnoreCase); - - private static readonly ConcurrentDictionary> PooledPEReaders = new(StringComparer.OrdinalIgnoreCase); - - private readonly string metadataPath; + private readonly MetadataFile metadataFile; private readonly Platform? platform; @@ -72,14 +58,14 @@ internal class MetadataIndex /// /// Initializes a new instance of the class. /// - /// The path to the metadata that this index will represent. + /// The metadata file that this index will represent. /// The platform filter to apply when reading the metadata. - private MetadataIndex(string metadataPath, Platform? platform) + internal MetadataIndex(MetadataFile metadataFile, Platform? platform) { - this.metadataPath = metadataPath; + this.metadataFile = metadataFile; this.platform = platform; - using Rental 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 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})"; /// /// Attempts to translate a to a . @@ -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 ss) { if (ss.Count == 0) @@ -497,33 +425,4 @@ private static string CommonPrefix(IReadOnlyList 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 - { - 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 index ef6b457d..1dd2db87 100644 --- a/src/Microsoft.Windows.CsWin32/SourceGenerator.cs +++ b/src/Microsoft.Windows.CsWin32/SourceGenerator.cs @@ -218,129 +218,122 @@ public void Execute(GeneratorExecutionContext context) return; } - SuperGenerator superGenerator = SuperGenerator.Combine(generators); - try + using SuperGenerator superGenerator = SuperGenerator.Combine(generators); + foreach (AdditionalText nativeMethodsTxtFile in nativeMethodsTxtFiles) { - foreach (AdditionalText nativeMethodsTxtFile in nativeMethodsTxtFiles) + SourceText? nativeMethodsTxt = nativeMethodsTxtFile.GetText(context.CancellationToken); + if (nativeMethodsTxt is null) { - SourceText? nativeMethodsTxt = nativeMethodsTxtFile.GetText(context.CancellationToken); - if (nativeMethodsTxt is null) + return; + } + + foreach (TextLine line in nativeMethodsTxt.Lines) + { + context.CancellationToken.ThrowIfCancellationRequested(); + string name = line.ToString(); + if (string.IsNullOrWhiteSpace(name) || name.StartsWith("//", StringComparison.InvariantCulture)) { - return; + continue; } - foreach (TextLine line in nativeMethodsTxt.Lines) + name = name.Trim().Trim(ZeroWhiteSpace); + var location = Location.Create(nativeMethodsTxtFile.Path, line.Span, nativeMethodsTxt.Lines.GetLinePositionSpan(line.Span)); + try { - context.CancellationToken.ThrowIfCancellationRequested(); - string name = line.ToString(); - if (string.IsNullOrWhiteSpace(name) || name.StartsWith("//", StringComparison.InvariantCulture)) + if (Generator.GetBannedAPIs(options).TryGetValue(name, out string? reason)) { + context.ReportDiagnostic(Diagnostic.Create(BannedApi, location, reason)); continue; } - name = name.Trim().Trim(ZeroWhiteSpace); - var location = Location.Create(nativeMethodsTxtFile.Path, line.Span, nativeMethodsTxt.Lines.GetLinePositionSpan(line.Span)); - try + if (name.EndsWith(".*", StringComparison.Ordinal)) { - if (Generator.GetBannedAPIs(options).TryGetValue(name, out string? reason)) - { - context.ReportDiagnostic(Diagnostic.Create(BannedApi, location, reason)); - continue; - } - - if (name.EndsWith(".*", StringComparison.Ordinal)) - { - string? moduleName = name.Substring(0, name.Length - 2); - int matches = superGenerator.TryGenerateAllExternMethods(moduleName, context.CancellationToken); - switch (matches) - { - case 0: - context.ReportDiagnostic(Diagnostic.Create(NoMethodsForModule, location, moduleName)); - break; - case > 1: - // Stop complaining about multiple metadata exporting methods from the same module. - // https://github.com/microsoft/CsWin32/issues/1201 - ////context.ReportDiagnostic(Diagnostic.Create(AmbiguousMatchError, location, moduleName)); - break; - } - - continue; - } - - superGenerator.TryGenerate(name, out IReadOnlyCollection matchingApis, out IReadOnlyCollection redirectedEnums, context.CancellationToken); - foreach (string declaringEnum in redirectedEnums) - { - context.ReportDiagnostic(Diagnostic.Create(UseEnumValueDeclaringType, location, declaringEnum)); - } - - switch (matchingApis.Count) + string? moduleName = name.Substring(0, name.Length - 2); + int matches = superGenerator.TryGenerateAllExternMethods(moduleName, context.CancellationToken); + switch (matches) { case 0: - ReportNoMatch(location, name); + context.ReportDiagnostic(Diagnostic.Create(NoMethodsForModule, location, moduleName)); break; case > 1: - context.ReportDiagnostic(Diagnostic.Create(AmbiguousMatchErrorWithSuggestions, location, name, ConcatSuggestions(matchingApis))); + // Stop complaining about multiple metadata exporting methods from the same module. + // https://github.com/microsoft/CsWin32/issues/1201 + ////context.ReportDiagnostic(Diagnostic.Create(AmbiguousMatchError, location, moduleName)); break; } + + continue; } - catch (GenerationFailedException ex) + + superGenerator.TryGenerate(name, out IReadOnlyCollection matchingApis, out IReadOnlyCollection redirectedEnums, context.CancellationToken); + foreach (string declaringEnum in redirectedEnums) { - if (Generator.IsPlatformCompatibleException(ex)) - { - context.ReportDiagnostic(Diagnostic.Create(CpuArchitectureIncompatibility, location)); - } - else - { - // Build up a complete error message. - context.ReportDiagnostic(Diagnostic.Create(InternalError, location, AssembleFullExceptionMessage(ex))); - } + context.ReportDiagnostic(Diagnostic.Create(UseEnumValueDeclaringType, location, declaringEnum)); } - } - } - - foreach (KeyValuePair unit in superGenerator.GetCompilationUnits(context.CancellationToken)) - { - context.AddSource(unit.Key, unit.Value.GetText(Encoding.UTF8)); - } - string ConcatSuggestions(IReadOnlyCollection suggestions) - { - var suggestionBuilder = new StringBuilder(); - int i = 0; - foreach (string suggestion in suggestions) + switch (matchingApis.Count) + { + case 0: + ReportNoMatch(location, name); + break; + case > 1: + context.ReportDiagnostic(Diagnostic.Create(AmbiguousMatchErrorWithSuggestions, location, name, ConcatSuggestions(matchingApis))); + break; + } + } + catch (GenerationFailedException ex) { - if (++i > 0) + if (Generator.IsPlatformCompatibleException(ex)) { - suggestionBuilder.Append(i < suggestions.Count - 1 ? ", " : " or "); + context.ReportDiagnostic(Diagnostic.Create(CpuArchitectureIncompatibility, location)); + } + else + { + // Build up a complete error message. + context.ReportDiagnostic(Diagnostic.Create(InternalError, location, AssembleFullExceptionMessage(ex))); } - - suggestionBuilder.Append('"'); - suggestionBuilder.Append(suggestion); - suggestionBuilder.Append('"'); } - - return suggestionBuilder.ToString(); } + } + + foreach (KeyValuePair unit in superGenerator.GetCompilationUnits(context.CancellationToken)) + { + context.AddSource(unit.Key, unit.Value.GetText(Encoding.UTF8)); + } - void ReportNoMatch(Location? location, string failedAttempt) + string ConcatSuggestions(IReadOnlyCollection suggestions) + { + var suggestionBuilder = new StringBuilder(); + int i = 0; + foreach (string suggestion in suggestions) { - IReadOnlyList suggestions = superGenerator.GetSuggestions(failedAttempt); - if (suggestions.Count > 0) + if (++i > 0) { - context.ReportDiagnostic(Diagnostic.Create(NoMatchingMethodOrTypeWithSuggestions, location, failedAttempt, ConcatSuggestions(suggestions))); - } - else - { - context.ReportDiagnostic(Diagnostic.Create( - Generator.ContainsIllegalCharactersForAPIName(failedAttempt) ? NoMatchingMethodOrTypeWithBadCharacters : NoMatchingMethodOrType, - location, - failedAttempt)); + suggestionBuilder.Append(i < suggestions.Count - 1 ? ", " : " or "); } + + suggestionBuilder.Append('"'); + suggestionBuilder.Append(suggestion); + suggestionBuilder.Append('"'); } + + return suggestionBuilder.ToString(); } - finally + + void ReportNoMatch(Location? location, string failedAttempt) { - superGenerator.Dispose(); + IReadOnlyList suggestions = superGenerator.GetSuggestions(failedAttempt); + if (suggestions.Count > 0) + { + context.ReportDiagnostic(Diagnostic.Create(NoMatchingMethodOrTypeWithSuggestions, location, failedAttempt, ConcatSuggestions(suggestions))); + } + else + { + context.ReportDiagnostic(Diagnostic.Create( + Generator.ContainsIllegalCharactersForAPIName(failedAttempt) ? NoMatchingMethodOrTypeWithBadCharacters : NoMatchingMethodOrType, + location, + failedAttempt)); + } } } diff --git a/test/Microsoft.Windows.CsWin32.Tests/GeneratorTestBase.cs b/test/Microsoft.Windows.CsWin32.Tests/GeneratorTestBase.cs index e6342990..fa476cf7 100644 --- a/test/Microsoft.Windows.CsWin32.Tests/GeneratorTestBase.cs +++ b/test/Microsoft.Windows.CsWin32.Tests/GeneratorTestBase.cs @@ -129,7 +129,11 @@ void AddSymbols(params string[] symbols) this.compilation = this.starterCompilations[DefaultTFM]; } - public ValueTask DisposeAsync() => ValueTask.CompletedTask; + public ValueTask DisposeAsync() + { + this.Dispose(); + return ValueTask.CompletedTask; + } public void Dispose() {