Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Jul 7, 2022
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5fe3341
Switch to a callback action
CyrusNajmabadi Jun 22, 2022
40c7d88
Merge remote-tracking branch 'upstream/main' into disposeAction
CyrusNajmabadi Jul 5, 2022
dc55787
Update comment
CyrusNajmabadi Jul 5, 2022
8026b4f
simplify
CyrusNajmabadi Jul 5, 2022
b893a60
Merge remote-tracking branch 'upstream/main' into disposeAction
CyrusNajmabadi Jul 7, 2022
e0a68f3
Fix comment
CyrusNajmabadi Jul 7, 2022
addafae
Simplify approach
CyrusNajmabadi Jul 7, 2022
c767714
Update src/Compilers/Core/Portable/MetadataReference/ModuleMetadata.cs
CyrusNajmabadi Jul 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/Compilers/Core/CodeAnalysisTest/MetadataReferences/ModuleMetadataTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
91 changes: 45 additions & 46 deletions src/Compilers/Core/Portable/MetadataReference/ModuleMetadata.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,58 +22,58 @@ 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;
private readonly Action? _onDispose;

/// <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)"/>.
/// Whether or not to call _onDispose when this is disposed. The reason that this is controlled by a flag is
/// that if we make a copy of this modulemetadata, we still want to keep the Action alive in it (as it may be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modulemetadata

Minor: "ModuleMetadata"

/// keeping alive underlying object this metadata needs). However, the copy of the metadata should not call the
/// action as only the <see cref="Metadata.IsImageOwner"/> metadata is responsible for that.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems dangerous to me. Who says that the copy has a shorter lifetime than the original? Feels like we need some sort of reference count wrapper that is decremented on dispose of the containing reference.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree ti's dangerous. BUt it's already the established semantics of ModuleMetadta. e.g. prior to this change if you did this:

ModuleMetadata mm = ... 
mmCopy = mm.Copy();
mm.Dispose();

then mmCopy was now in an 'no longer usable' state (same with any other copies created). I'm genuinely surprised we went with those semantics, but it's been the way of this type since the beginning, so it's just part of the nature of these and hte entire stack is written to be ok with that.

I would totally be ok with moving to a refcounted system. And, importantly, it would not change this new API at all. All it would do would be to allow us to remove those comments and make things safer. As such, i would prefer it be a separate orthogonal API :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm genuinely surprised we went with those semantics,

For performance reasons. Entirely safe semantics would be slow since every read from the metadata would need to check whether the memory is disposed or not before it reads and do these two operations atomically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who says that the copy has a shorter lifetime than the original?

It's responsibility of the owner of the metadata to guarantee that.

Copy link
Member

@tmat tmat Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be possible to make metadata reads safe using ReadOnlySpan<T> and ReadOnlyMemory<T>. But that would require a lot of changes throughout SRM and also might not be sufficiently efficient on .NET Framework.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess making the copies ref-counted would add some value. It wouldn't make it entirely safe but perhaps safer.

/// </summary>
private readonly bool _disposeOwner;
private readonly bool _callOnDispose;

private bool _isDisposed;

private ModuleMetadata(PEReader peReader, IDisposable? owner, bool disposeOwner)
private ModuleMetadata(PEReader peReader, Action? onDispose, bool callOnDispose)
: 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);
Debug.Assert(!callOnDispose || onDispose is not null);

_module = new PEModule(this, peReader: peReader, metadataOpt: IntPtr.Zero, metadataSizeOpt: 0, includeEmbeddedInteropTypes: false, ignoreAssemblyRefs: false);
_owner = owner;
_disposeOwner = disposeOwner;
_onDispose = onDispose;
_callOnDispose = callOnDispose;
}

private ModuleMetadata(
IntPtr metadata,
int size,
IDisposable? owner,
bool disposeOwner,
Action? onDispose,
bool callOnDispose,
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);
Debug.Assert(!callOnDispose || onDispose is not null);

_module = new PEModule(this, peReader: null, metadataOpt: metadata, metadataSizeOpt: size, includeEmbeddedInteropTypes: includeEmbeddedInteropTypes, ignoreAssemblyRefs: ignoreAssemblyRefs);
_owner = owner;
_disposeOwner = disposeOwner;
_onDispose = onDispose;
_callOnDispose = callOnDispose;
}

// 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;

// Keep the onDispose callback alive as it may be rooting data that this metadata needs. However, mark that
// we are not to dispose it ourselves, that's only the responsibility of the image owner (see IsImageOwner).
_onDispose = metadata._onDispose;
_callOnDispose = false;
}

/// <summary>
Expand All @@ -85,37 +85,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)
{
Expand All @@ -127,14 +123,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, callOnDispose: onDispose != null, 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, callOnDispose: false, includeEmbeddedInteropTypes, ignoreAssemblyRefs);
}

/// <summary>
Expand All @@ -145,9 +141,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)
{
Expand All @@ -159,7 +155,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, callOnDispose: onDispose != null);
}

/// <summary>
Expand Down Expand Up @@ -189,7 +185,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, callOnDispose: false);
}

/// <summary>
Expand Down Expand Up @@ -249,11 +245,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);
}
}

Expand All @@ -265,7 +264,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, callOnDispose: false);
}

/// <summary>
Expand Down Expand Up @@ -317,8 +316,8 @@ public override void Dispose()
{
_module.Dispose();

if (_disposeOwner)
_owner!.Dispose();
if (_callOnDispose)
_onDispose!.Invoke();
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/Portable/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this new public API. Makes me happy :).

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!
Expand Down
2 changes: 1 addition & 1 deletion ...MetadataReferences/VisualStudioMetadataReferenceManager.RecoverableMetadataValueSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...lStudio/Core/Def/ProjectSystem/MetadataReferences/VisualStudioMetadataReferenceManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/Workspaces/Core/Portable/Serialization/SerializerService_Reference.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down