Switch to a callback for cleaning up resources instead of passing in an explicit IDisposable.

src/Compilers/Core/CodeAnalysisTest/MetadataReferences/ModuleMetadataTests.cs

@@ -64,7 +64,7 @@ public unsafe void CreateFromMetadata_Assembly_Stream()
             fixed (byte* ptr = &assembly[h.MetadataStartOffset])
             {
                 var stream = new UnmanagedMemoryStream(ptr, h.MetadataSize);
-                var metadata = ModuleMetadata.CreateFromMetadata((IntPtr)stream.PositionPointer, (int)stream.Length, stream, disposeOwner: true);
+                var metadata = ModuleMetadata.CreateFromMetadata((IntPtr)stream.PositionPointer, (int)stream.Length, stream.Dispose);
                 Assert.Equal(new AssemblyIdentity("Members"), metadata.Module.ReadAssemblyIdentityOrThrow());
             }
         }
@@ -78,7 +78,7 @@ public unsafe void CreateFromMetadata_Module_Stream()
             fixed (byte* ptr = &netModule[h.MetadataStartOffset])
             {
                 var stream = new UnmanagedMemoryStream(ptr, h.MetadataSize);
-                ModuleMetadata.CreateFromMetadata((IntPtr)stream.PositionPointer, (int)stream.Length, stream, disposeOwner: true);
+                ModuleMetadata.CreateFromMetadata((IntPtr)stream.PositionPointer, (int)stream.Length, stream.Dispose);
             }
         }
 
@@ -214,7 +214,7 @@ public unsafe void CreateFromMetadata_Assembly_Stream_DisposeOwnerTrue()
                     OnSeek = (_, _) => seeked = true,
                 };
 
-                var metadata = ModuleMetadata.CreateFromMetadata((IntPtr)stream.PositionPointer, (int)stream.Length, stream, disposeOwner: true);
+                var metadata = ModuleMetadata.CreateFromMetadata((IntPtr)stream.PositionPointer, (int)stream.Length, stream.Dispose);
                 Assert.Equal(new AssemblyIdentity("Members"), metadata.Module.ReadAssemblyIdentityOrThrow());
 
                 // Disposing the metadata should dispose the stream.
@@ -272,7 +272,7 @@ public unsafe void CreateFromMetadata_Assembly_Stream_DisposeOwnerFalse()
                     OnSeek = (_, _) => seeked = true,
                 };
 
-                var metadata = ModuleMetadata.CreateFromMetadata((IntPtr)stream.PositionPointer, (int)stream.Length, stream, disposeOwner: false);
+                var metadata = ModuleMetadata.CreateFromMetadata((IntPtr)stream.PositionPointer, (int)stream.Length);
                 Assert.Equal(new AssemblyIdentity("Members"), metadata.Module.ReadAssemblyIdentityOrThrow());
 
                 // Disposing the metadata should not dispose the stream.

src/Compilers/Core/Portable/MetadataReference/ModuleMetadata.cs

@@ -9,6 +9,7 @@
 using System.IO;
 using System.Reflection.Metadata;
 using System.Reflection.PortableExecutable;
+using System.Threading;
 using Roslyn.Utilities;
 
 namespace Microsoft.CodeAnalysis
@@ -22,58 +23,41 @@ public sealed partial class ModuleMetadata : Metadata
         private readonly PEModule _module;
 
         /// <summary>
-        /// Optional data that should be kept alive as long as this <see cref="ModuleMetadata"/> is alive.  This can be
-        /// useful, for example, if there is backing memory that the metadata depends on that should be kept rooted so it
-        /// doesn't get garbage collected.
+        /// Optional action to invoke when this metadata is disposed.
         /// </summary>
-        private readonly IDisposable? _owner;
-
-        /// <summary>
-        /// Whether or not <see cref="_owner"/> should be <see cref="IDisposable.Dispose"/>'d when this object is
-        /// Disposed.  Is controlled by the <c>leaveOpen</c> flag in <see cref="CreateFromStream(Stream, bool)"/>, or
-        /// the <see cref="PEStreamOptions.LeaveOpen"/> flag in <see cref="CreateFromStream(Stream, PEStreamOptions)"/>.
-        /// </summary>
-        private readonly bool _disposeOwner;
+        private Action? _onDispose;
 
         private bool _isDisposed;
 
-        private ModuleMetadata(PEReader peReader, IDisposable? owner, bool disposeOwner)
+        private ModuleMetadata(PEReader peReader, Action? onDispose)
             : base(isImageOwner: true, id: MetadataId.CreateNewId())
         {
-            // If we've been asked to dispose the owner, then we better have an owner to dispose.
-            Debug.Assert(!disposeOwner || owner is not null);
-
             _module = new PEModule(this, peReader: peReader, metadataOpt: IntPtr.Zero, metadataSizeOpt: 0, includeEmbeddedInteropTypes: false, ignoreAssemblyRefs: false);
-            _owner = owner;
-            _disposeOwner = disposeOwner;
+            _onDispose = onDispose;
         }
 
         private ModuleMetadata(
             IntPtr metadata,
             int size,
-            IDisposable? owner,
-            bool disposeOwner,
+            Action? onDispose,
             bool includeEmbeddedInteropTypes,
             bool ignoreAssemblyRefs)
             : base(isImageOwner: true, id: MetadataId.CreateNewId())
         {
-            // If we've been asked to dispose the owner, then we better have an owner to dispose.
-            Debug.Assert(!disposeOwner || owner is not null);
-
             _module = new PEModule(this, peReader: null, metadataOpt: metadata, metadataSizeOpt: size, includeEmbeddedInteropTypes: includeEmbeddedInteropTypes, ignoreAssemblyRefs: ignoreAssemblyRefs);
-            _owner = owner;
-            _disposeOwner = disposeOwner;
+            _onDispose = onDispose;
         }
 
         // creates a copy
         private ModuleMetadata(ModuleMetadata metadata)
             : base(isImageOwner: false, id: metadata.Id)
         {
             _module = metadata.Module;
-            // ensure that we keep the owner rooted so that it can't get GC'ed why we're alive.
-            _owner = metadata._owner;
-            // however, as we're not the image owner, we will never dispose the owner.  Only the single image owner can be responsible for that.
-            _disposeOwner = false;
+
+            // note: we intentionally do not pass the _onDispose callback to the copy.  Only the owner owns the callback
+            // and controls calling it.  This does mean that the callback (and underlying memory it holds onto) may
+            // disappear once the owner is disposed or GC'd.  But that's ok as that is expected semantics.  Once an image
+            // owner is gone, all copies are no longer in a valid state for use.
         }
 
         /// <summary>
@@ -85,37 +69,33 @@ private ModuleMetadata(ModuleMetadata metadata)
         /// <exception cref="ArgumentNullException"><paramref name="metadata"/> is null.</exception>
         /// <exception cref="ArgumentOutOfRangeException"><paramref name="size"/> is not positive.</exception>
         public static ModuleMetadata CreateFromMetadata(IntPtr metadata, int size)
-            => CreateFromMetadataWorker(metadata, size, owner: null, disposeOwner: false);
+            => CreateFromMetadataWorker(metadata, size, onDispose: null);
 
         /// <summary>
         /// Create metadata module from a raw memory pointer to metadata directory of a PE image or .cormeta section of an object file.
         /// Only manifest modules are currently supported.
         /// </summary>
         /// <param name="metadata">Pointer to the start of metadata block.</param>
         /// <param name="size">The size of the metadata block.</param>
-        /// <param name="owner">Data that should be kept alive as long as this <see cref="ModuleMetadata"/> is alive.  This can be
-        /// useful, for example, if there is backing memory that the metadata depends on that should be kept rooted so it
-        /// doesn't get garbage collected.</param>
-        /// <param name="disposeOwner">Whether or not <paramref name="owner"/> should be <see cref="IDisposable.Dispose"/>'d when this object is
-        /// Disposed.</param>
-        /// <exception cref="ArgumentNullException"><paramref name="owner"/> is null.</exception>
+        /// <param name="onDispose">Action to run when the metadata module is disposed.  This will only be called then
+        /// this actual metadata instance is disposed.  Any instances created from this using <see
+        /// cref="Metadata.Copy"/> will not call this when they are disposed.</param>
+        /// <exception cref="ArgumentNullException"><paramref name="onDispose"/> is null.</exception>
         public static unsafe ModuleMetadata CreateFromMetadata(
             IntPtr metadata,
             int size,
-            IDisposable owner,
-            bool disposeOwner)
+            Action onDispose)
         {
-            if (owner is null)
-                throw new ArgumentNullException(nameof(owner));
+            if (onDispose is null)
+                throw new ArgumentNullException(nameof(onDispose));
 
-            return CreateFromMetadataWorker(metadata, size, owner, disposeOwner);
+            return CreateFromMetadataWorker(metadata, size, onDispose);
         }
 
         private static ModuleMetadata CreateFromMetadataWorker(
             IntPtr metadata,
             int size,
-            IDisposable? owner,
-            bool disposeOwner)
+            Action? onDispose)
         {
             if (metadata == IntPtr.Zero)
             {
@@ -127,14 +107,14 @@ private static ModuleMetadata CreateFromMetadataWorker(
                 throw new ArgumentOutOfRangeException(CodeAnalysisResources.SizeHasToBePositive, nameof(size));
             }
 
-            return new ModuleMetadata(metadata, size, owner, disposeOwner, includeEmbeddedInteropTypes: false, ignoreAssemblyRefs: false);
+            return new ModuleMetadata(metadata, size, onDispose, includeEmbeddedInteropTypes: false, ignoreAssemblyRefs: false);
         }
 
         internal static ModuleMetadata CreateFromMetadata(IntPtr metadata, int size, bool includeEmbeddedInteropTypes, bool ignoreAssemblyRefs = false)
         {
             Debug.Assert(metadata != IntPtr.Zero);
             Debug.Assert(size > 0);
-            return new ModuleMetadata(metadata, size, owner: null, disposeOwner: false, includeEmbeddedInteropTypes, ignoreAssemblyRefs);
+            return new ModuleMetadata(metadata, size, onDispose: null, includeEmbeddedInteropTypes, ignoreAssemblyRefs);
         }
 
         /// <summary>
@@ -145,9 +125,9 @@ internal static ModuleMetadata CreateFromMetadata(IntPtr metadata, int size, boo
         /// <exception cref="ArgumentNullException"><paramref name="peImage"/> is null.</exception>
         /// <exception cref="ArgumentOutOfRangeException"><paramref name="size"/> is not positive.</exception>
         public static unsafe ModuleMetadata CreateFromImage(IntPtr peImage, int size)
-            => CreateFromImage((byte*)peImage, size, owner: null, disposeOwner: false);
+            => CreateFromImage((byte*)peImage, size, onDispose: null);
 
-        private static unsafe ModuleMetadata CreateFromImage(byte* peImage, int size, IDisposable? owner, bool disposeOwner)
+        private static unsafe ModuleMetadata CreateFromImage(byte* peImage, int size, Action? onDispose)
         {
             if (peImage == null)
             {
@@ -159,7 +139,7 @@ private static unsafe ModuleMetadata CreateFromImage(byte* peImage, int size, ID
                 throw new ArgumentOutOfRangeException(CodeAnalysisResources.SizeHasToBePositive, nameof(size));
             }
 
-            return new ModuleMetadata(new PEReader(peImage, size), owner, disposeOwner);
+            return new ModuleMetadata(new PEReader(peImage, size), onDispose);
         }
 
         /// <summary>
@@ -189,7 +169,7 @@ public static ModuleMetadata CreateFromImage(ImmutableArray<byte> peImage)
                 throw new ArgumentNullException(nameof(peImage));
             }
 
-            return new ModuleMetadata(new PEReader(peImage), owner: null, disposeOwner: false);
+            return new ModuleMetadata(new PEReader(peImage), onDispose: null);
         }
 
         /// <summary>
@@ -249,11 +229,14 @@ public static ModuleMetadata CreateFromStream(Stream peStream, PEStreamOptions o
             {
                 unsafe
                 {
+                    Action? onDispose = options.HasFlag(PEStreamOptions.LeaveOpen)
+                        ? null
+                        : unmanagedMemoryStream.Dispose;
+
                     return CreateFromImage(
                         unmanagedMemoryStream.PositionPointer,
                         (int)Math.Min(unmanagedMemoryStream.Length, int.MaxValue),
-                        owner: unmanagedMemoryStream,
-                        disposeOwner: !options.HasFlag(PEStreamOptions.LeaveOpen));
+                        onDispose);
                 }
             }
 
@@ -265,7 +248,7 @@ public static ModuleMetadata CreateFromStream(Stream peStream, PEStreamOptions o
             }
 
             // ownership of the stream is passed on PEReader:
-            return new ModuleMetadata(new PEReader(peStream, options), owner: null, disposeOwner: false);
+            return new ModuleMetadata(new PEReader(peStream, options), onDispose: null);
         }
 
         /// <summary>
@@ -317,8 +300,8 @@ public override void Dispose()
             {
                 _module.Dispose();
 
-                if (_disposeOwner)
-                    _owner!.Dispose();
+                var onDispose = Interlocked.Exchange(ref _onDispose, null);
+                onDispose?.Invoke();
             }
         }
 

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

@@ -76,7 +76,7 @@ override sealed Microsoft.CodeAnalysis.CompilationOptions.GetHashCode() -> int
 override sealed Microsoft.CodeAnalysis.Diagnostic.Equals(object? obj) -> bool
 *REMOVED*static Microsoft.CodeAnalysis.SyntaxNodeExtensions.ReplaceSyntax<TRoot>(this TRoot! root, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.SyntaxNode!>! nodes, System.Func<Microsoft.CodeAnalysis.SyntaxNode!, Microsoft.CodeAnalysis.SyntaxNode!, Microsoft.CodeAnalysis.SyntaxNode!>! computeReplacementNode, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.SyntaxToken>! tokens, System.Func<Microsoft.CodeAnalysis.SyntaxToken, Microsoft.CodeAnalysis.SyntaxToken, Microsoft.CodeAnalysis.SyntaxToken>! computeReplacementToken, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.SyntaxTrivia>! trivia, System.Func<Microsoft.CodeAnalysis.SyntaxTrivia, Microsoft.CodeAnalysis.SyntaxTrivia, Microsoft.CodeAnalysis.SyntaxTrivia>! computeReplacementTrivia) -> TRoot!
 static Microsoft.CodeAnalysis.Emit.EditAndContinueMethodDebugInformation.Create(System.Collections.Immutable.ImmutableArray<byte> compressedSlotMap, System.Collections.Immutable.ImmutableArray<byte> compressedLambdaMap, System.Collections.Immutable.ImmutableArray<byte> compressedStateMachineStateMap) -> Microsoft.CodeAnalysis.Emit.EditAndContinueMethodDebugInformation
-static Microsoft.CodeAnalysis.ModuleMetadata.CreateFromMetadata(System.IntPtr metadata, int size, System.IDisposable! owner, bool disposeOwner) -> Microsoft.CodeAnalysis.ModuleMetadata!
+static Microsoft.CodeAnalysis.ModuleMetadata.CreateFromMetadata(System.IntPtr metadata, int size, System.Action! onDispose) -> Microsoft.CodeAnalysis.ModuleMetadata!
 static Microsoft.CodeAnalysis.SyntaxNodeExtensions.ReplaceSyntax<TRoot>(this TRoot! root, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.SyntaxNode!>? nodes, System.Func<Microsoft.CodeAnalysis.SyntaxNode!, Microsoft.CodeAnalysis.SyntaxNode!, Microsoft.CodeAnalysis.SyntaxNode!>? computeReplacementNode, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.SyntaxToken>? tokens, System.Func<Microsoft.CodeAnalysis.SyntaxToken, Microsoft.CodeAnalysis.SyntaxToken, Microsoft.CodeAnalysis.SyntaxToken>? computeReplacementToken, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.SyntaxTrivia>? trivia, System.Func<Microsoft.CodeAnalysis.SyntaxTrivia, Microsoft.CodeAnalysis.SyntaxTrivia, Microsoft.CodeAnalysis.SyntaxTrivia>? computeReplacementTrivia) -> TRoot!
 const Microsoft.CodeAnalysis.WellKnownMemberNames.CheckedDecrementOperatorName = "op_CheckedDecrement" -> string!
 const Microsoft.CodeAnalysis.WellKnownMemberNames.CheckedIncrementOperatorName = "op_CheckedIncrement" -> string!

src/VisualStudio/Core/Def/ProjectSystem/MetadataReferences/VisualStudioMetadataReferenceManager.RecoverableMetadataValueSource.cs

@@ -80,7 +80,7 @@ private static ModuleMetadata GetModuleMetadata(
                 var stream = storage.ReadStream(CancellationToken.None);
                 unsafe
                 {
-                    return ModuleMetadata.CreateFromMetadata((IntPtr)stream.PositionPointer, (int)stream.Length, stream, disposeOwner: true);
+                    return ModuleMetadata.CreateFromMetadata((IntPtr)stream.PositionPointer, (int)stream.Length, stream.Dispose);
                 }
             }
         }

src/VisualStudio/Core/Def/ProjectSystem/MetadataReferences/VisualStudioMetadataReferenceManager.cs

@@ -168,7 +168,7 @@ private ModuleMetadata CreateModuleMetadataFromTemporaryStorage(
             unsafe
             {
                 // For an unmanaged memory stream, ModuleMetadata can take ownership directly.
-                var metadata = ModuleMetadata.CreateFromMetadata((IntPtr)stream.PositionPointer, (int)stream.Length, stream, disposeOwner: true);
+                var metadata = ModuleMetadata.CreateFromMetadata((IntPtr)stream.PositionPointer, (int)stream.Length, stream.Dispose);
 
                 // hold onto storage if requested
                 storages?.Add(storage);

src/Workspaces/Core/Portable/Serialization/SerializerService_Reference.cs

@@ -496,7 +496,7 @@ private static void GetMetadata(Stream stream, long length, out ModuleMetadata m
                 unsafe
                 {
                     metadata = ModuleMetadata.CreateFromMetadata(
-                        (IntPtr)unmanagedStream.PositionPointer, (int)unmanagedStream.Length, unmanagedStream, disposeOwner: true);
+                        (IntPtr)unmanagedStream.PositionPointer, (int)unmanagedStream.Length, unmanagedStream.Dispose);
                     lifeTimeObject = null;
                     return;
                 }