Make DependentHandle public #54246
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,253 @@ | ||||||||
| // Licensed to the .NET Foundation under one or more agreements. | ||||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||||
|
|
||||||||
| using System.Runtime.CompilerServices; | ||||||||
| #if !DEBUG | ||||||||
| using Internal.Runtime.CompilerServices; | ||||||||
| #endif | ||||||||
|
|
||||||||
| namespace System.Runtime | ||||||||
| { | ||||||||
| /// <summary> | ||||||||
| /// Represents a dependent GC handle, which will conditionally keep a dependent object instance alive | ||||||||
| /// as long as a target object instance is alive as well, without representing a strong reference to the | ||||||||
| /// target object instance. That is, a <see cref="DependentHandle"/> value with a given object instance as | ||||||||
| /// target will not cause the target to be kept alive if there are no other strong references to it, but | ||||||||
| /// it will do so for the dependent object instance as long as the target is alive. | ||||||||
| /// <para> | ||||||||
| /// This type is conceptually equivalent to having a weak reference to a given target object instance A, with | ||||||||
| /// that object having a field or property (or some other strong reference) to a dependent object instance B. | ||||||||
| /// </para> | ||||||||
| /// </summary> | ||||||||
| /// <remarks> | ||||||||
| /// The <see cref="DependentHandle"/> type is not thread-safe, and consumers are responsible for ensuring that | ||||||||
| /// <see cref="Dispose"/> is not called concurrently with other APIs. Not doing so results in undefined behavior. | ||||||||
| /// <para>The <see cref="Target"/> and <see cref="Dependent"/> properties are instead thread-safe.</para> | ||||||||
| /// </remarks> | ||||||||
| public struct DependentHandle : IDisposable | ||||||||
|
There was a problem hiding this comment. Should we add a [DebuggerDisplay(...)] attribute? And/or a [DebuggerTypeProxy(...)]? There was a problem hiding this comment. Not sure how to structure them exactly, as in, what info did you have in mind for them to expose that wouldn't already be displayed by the built-in debugger view? I guess I can see how eg. EDIT: Tanner pointed me towards this comment from Jan that seems to suggest that it might not be worth it in this case to add either of those two debugging types, given that the debugger interfering with the GC is by design? 🤔 There was a problem hiding this comment. I was actually thinking more about, for example, the Target and Dependent properties showing as null instead of showing as a thrown exception if the instance isn't allocated. But, not a big deal. |
||||||||
| { | ||||||||
| // ========================================================================================= | ||||||||
| // This struct collects all operations on native DependentHandles. The DependentHandle | ||||||||
| // merely wraps an IntPtr so this struct serves mainly as a "managed typedef." | ||||||||
| // | ||||||||
| // DependentHandles exist in one of two states: | ||||||||
| // | ||||||||
| // IsAllocated == false | ||||||||
| // No actual handle is allocated underneath. Illegal to get Target, Dependent | ||||||||
| // or GetTargetAndDependent(). Ok to call Dispose(). | ||||||||
| // | ||||||||
| // Initializing a DependentHandle using the nullary ctor creates a DependentHandle | ||||||||
| // that's in the !IsAllocated state. | ||||||||
| // (! Right now, we get this guarantee for free because (IntPtr)0 == NULL unmanaged handle. | ||||||||
| // ! If that assertion ever becomes false, we'll have to add an _isAllocated field | ||||||||
| // ! to compensate.) | ||||||||
| // | ||||||||
| // | ||||||||
| // IsAllocated == true | ||||||||
| // There's a handle allocated underneath. You must call Dispose() on this eventually | ||||||||
| // or you cause a native handle table leak. | ||||||||
| // | ||||||||
| // This struct intentionally does no self-synchronization. It's up to the caller to | ||||||||
jkotas marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| // to use DependentHandles in a thread-safe way. | ||||||||
| // ========================================================================================= | ||||||||
|
|
||||||||
| private IntPtr _handle; | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Initializes a new instance of the <see cref="DependentHandle"/> struct with the specified arguments. | ||||||||
| /// </summary> | ||||||||
| /// <param name="target">The target object instance to track.</param> | ||||||||
| /// <param name="dependent">The dependent object instance to associate with <paramref name="target"/>.</param> | ||||||||
| public DependentHandle(object? target, object? dependent) | ||||||||
| { | ||||||||
| // no need to check for null result: nInitialize expected to throw OOM. | ||||||||
| _handle = InternalInitialize(target, dependent); | ||||||||
| } | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Gets a value indicating whether this handle has been allocated or not. | ||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||
| /// </summary> | ||||||||
| public bool IsAllocated => (nint)_handle != 0; | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Gets or sets the target object instance for the current handle. | ||||||||
| /// </summary> | ||||||||
| /// <exception cref="InvalidOperationException">Thrown if <see cref="IsAllocated"/> is <see langword="false"/>.</exception> | ||||||||
| /// <remarks>This property is thread-safe.</remarks> | ||||||||
|
There was a problem hiding this comment. IsAllocated is missing a similar thread-safety remark. |
||||||||
| public object? Target | ||||||||
| { | ||||||||
| get | ||||||||
| { | ||||||||
| IntPtr handle = _handle; | ||||||||
|
|
||||||||
| if ((nint)handle == 0) | ||||||||
| { | ||||||||
| ThrowHelper.ThrowInvalidOperationException(); | ||||||||
| } | ||||||||
|
|
||||||||
| return InternalGetTarget(handle); | ||||||||
| } | ||||||||
| set | ||||||||
| { | ||||||||
| IntPtr handle = _handle; | ||||||||
|
|
||||||||
| if ((nint)handle == 0) | ||||||||
| { | ||||||||
| ThrowHelper.ThrowInvalidOperationException(); | ||||||||
| } | ||||||||
|
|
||||||||
| InternalSetTarget(handle, value); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Gets or sets the dependent object instance for the current handle. | ||||||||
| /// </summary> | ||||||||
| /// <remarks> | ||||||||
| /// If it is needed to retrieve both <see cref="Target"/> and <see cref="Dependent"/>, it is necessary | ||||||||
| /// to ensure that the returned instance from <see cref="Target"/> will be kept alive until <see cref="Dependent"/> | ||||||||
| /// is retrieved as well, or it might be collected and result in unexpected behavior. This can be done by storing the | ||||||||
| /// target in a local and calling <see cref="GC.KeepAlive(object)"/> on it after <see cref="Dependent"/> is accessed. | ||||||||
| /// </remarks> | ||||||||
| /// <exception cref="InvalidOperationException">Thrown if <see cref="IsAllocated"/> is <see langword="false"/>.</exception> | ||||||||
| /// <remarks>This property is thread-safe.</remarks> | ||||||||
| public object? Dependent | ||||||||
| { | ||||||||
| get | ||||||||
| { | ||||||||
| IntPtr handle = _handle; | ||||||||
|
|
||||||||
| if ((nint)handle == 0) | ||||||||
| { | ||||||||
| ThrowHelper.ThrowInvalidOperationException(); | ||||||||
| } | ||||||||
|
|
||||||||
| return InternalGetDependent(handle); | ||||||||
| } | ||||||||
| set | ||||||||
| { | ||||||||
| IntPtr handle = _handle; | ||||||||
|
|
||||||||
| if ((nint)handle == 0) | ||||||||
| { | ||||||||
| ThrowHelper.ThrowInvalidOperationException(); | ||||||||
| } | ||||||||
|
|
||||||||
| InternalSetDependent(handle, value); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Atomically retrieves the values of both <see cref="Target"/> and <see cref="Dependent"/>, if available. | ||||||||
| /// That is, even if <see cref="Target"/> is concurrently set to <see langword="null"/>, calling this method | ||||||||
| /// will either return <see langword="null"/> for both target and dependent, or return both previous values. | ||||||||
| /// If <see cref="Target"/> and <see cref="Dependent"/> were used sequentially in this scenario instead, it's | ||||||||
| /// could be possible to sometimes successfully retrieve the previous target, but then fail to get the dependent. | ||||||||
| /// </summary> | ||||||||
| /// <returns>The values of <see cref="Target"/> and <see cref="Dependent"/>.</returns> | ||||||||
| /// <exception cref="InvalidOperationException">Thrown if <see cref="IsAllocated"/> is <see langword="false"/>.</exception> | ||||||||
|
|
||||||||
| public (object? Target, object? Dependent) GetTargetAndDependent() | ||||||||
|
There was a problem hiding this comment. I couldn't attend the API review for this... what was the reason this design was decided on rather than: public (object? Target, object? Dependent) TargetAndDependent { get; }? There was a problem hiding this comment. Jeremy mentioned that given that the method returned a tuple, it felt much more appropriate to him for it to be a method instead. I agree with that as well, and I'll also add that a tuple-returning property has never been used before in the BCL, whereas a tuple-returning method is more common today (eg. all the new There was a problem hiding this comment.
Why? We have many properties that return structs with multiple properties. What makes a ValueTuple special? There was a problem hiding this comment. Can't talk for Jeremy specifically, what he said on stream was:
For more on his part, you'd have to ask him 🙂 On my end, I can say that one of the reasons why I really feel like it should be a method is that in this case we're not conceptually returning an entity (such as a color, which has multiple channels), but two separate entities. The C# guideline often refer to properties as each representing "one entity", which is the case here as well, with There was a problem hiding this comment. Maybe we should just drop this method/property. The motivation for this method was that it helps to avoid certain types of race conditions: #19459 (comment) . These race conditions can be trivially avoided by the caller using This type of race conditions should be mentioned in the documentation in any case. Given that this is advanced type, I do not think that there is a huge difference between recomending to use There was a problem hiding this comment. Yup, was just going through the VM trying to understand exactly how that hack actually worked 😄 There was a problem hiding this comment. @jkotas After checking it looks like that with Andy's recent jump threading changes (#46257), the JIT can now correctly skip the second There was a problem hiding this comment. Yes, I would vote to remove the tuple returning APIs and just document the race conditions to be careful about. There was a problem hiding this comment. Removed in 9c23a947ce5da21fb6169b6dae16a38f1412e8e9 and added the documentation about There was a problem hiding this comment. I still see it in the PR. Did you intend to remove it? |
||||||||
| { | ||||||||
| IntPtr handle = _handle; | ||||||||
|
|
||||||||
| if ((nint)handle == 0) | ||||||||
| { | ||||||||
| ThrowHelper.ThrowInvalidOperationException(); | ||||||||
| } | ||||||||
|
|
||||||||
| object? target = InternalGetTargetAndDependent(handle, out object? dependent); | ||||||||
|
|
||||||||
| return (target, dependent); | ||||||||
| } | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Gets the target object instance for the current handle. | ||||||||
| /// </summary> | ||||||||
| /// <returns>The target object instance, if present.</returns> | ||||||||
| /// <remarks>This method mirrors <see cref="Target"/>, but without the allocation check.</remarks> | ||||||||
| internal object? UnsafeGetTarget() | ||||||||
| { | ||||||||
| return InternalGetTarget(_handle); | ||||||||
| } | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Atomically retrieves the values of both <see cref="Target"/> and <see cref="Dependent"/>, if available. | ||||||||
| /// </summary> | ||||||||
| /// <param name="dependent">The dependent instance, if available.</param> | ||||||||
| /// <returns>The values of <see cref="Target"/> and <see cref="Dependent"/>.</returns> | ||||||||
| /// <remarks> | ||||||||
| /// This method mirrors <see cref="GetTargetAndDependent"/>, but without the allocation check. | ||||||||
| /// The signature is also kept the same as the one for the internal call, to improve the codegen. | ||||||||
| /// Note that <paramref name="dependent"/> is required to be on the stack (or it might not be tracked). | ||||||||
| /// </remarks> | ||||||||
| internal object? UnsafeGetTargetAndDependent(out object? dependent) | ||||||||
| { | ||||||||
| return InternalGetTargetAndDependent(_handle, out dependent); | ||||||||
| } | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Sets the target object instance for the current handle. | ||||||||
| /// </summary> | ||||||||
| /// <remarks>This method mirrors <see cref="Target"/>, but without the allocation check.</remarks> | ||||||||
| internal void UnsafeSetTarget(object? target) | ||||||||
| { | ||||||||
| InternalSetTarget(_handle, target); | ||||||||
| } | ||||||||
|
|
||||||||
| /// <summary> | ||||||||
| /// Sets the dependent object instance for the current handle. | ||||||||
| /// </summary> | ||||||||
| /// <remarks>This method mirrors <see cref="Dependent"/>, but without the allocation check.</remarks> | ||||||||
| internal void UnsafeSetDependent(object? dependent) | ||||||||
| { | ||||||||
| InternalSetDependent(_handle, dependent); | ||||||||
| } | ||||||||
|
|
||||||||
| /// <inheritdoc cref="IDisposable.Dispose"/> | ||||||||
| /// <remarks>This method is not thread-safe.</remarks> | ||||||||
| public void Dispose() | ||||||||
| { | ||||||||
| // Forces the DependentHandle back to non-allocated state | ||||||||
| // (if not already there) and frees the handle if needed. | ||||||||
| IntPtr handle = _handle; | ||||||||
|
|
||||||||
| if ((nint)handle != 0) | ||||||||
| { | ||||||||
| _handle = IntPtr.Zero; | ||||||||
|
|
||||||||
| InternalFree(handle); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| [MethodImpl(MethodImplOptions.InternalCall)] | ||||||||
| private static extern IntPtr InternalInitialize(object? target, object? dependent); | ||||||||
|
|
||||||||
| #if DEBUG | ||||||||
| [MethodImpl(MethodImplOptions.InternalCall)] | ||||||||
| private static extern object? InternalGetTarget(IntPtr dependentHandle); | ||||||||
| #else | ||||||||
| private static unsafe object? InternalGetTarget(IntPtr dependentHandle) | ||||||||
| { | ||||||||
| // This optimization is the same that is used in GCHandle in RELEASE mode. | ||||||||
| // This is not used in DEBUG builds as the runtime performs additional checks. | ||||||||
| // The logic below is the inlined copy of ObjectFromHandle in the unmanaged runtime. | ||||||||
| return Unsafe.As<IntPtr, object>(ref *(IntPtr*)(nint)dependentHandle); | ||||||||
| } | ||||||||
| #endif | ||||||||
|
|
||||||||
| [MethodImpl(MethodImplOptions.InternalCall)] | ||||||||
| private static extern object? InternalGetDependent(IntPtr dependentHandle); | ||||||||
|
|
||||||||
| [MethodImpl(MethodImplOptions.InternalCall)] | ||||||||
| private static extern object? InternalGetTargetAndDependent(IntPtr dependentHandle, out object? dependent); | ||||||||
|
|
||||||||
| [MethodImpl(MethodImplOptions.InternalCall)] | ||||||||
| private static extern void InternalSetTarget(IntPtr dependentHandle, object? target); | ||||||||
|
|
||||||||
| [MethodImpl(MethodImplOptions.InternalCall)] | ||||||||
| private static extern void InternalSetDependent(IntPtr dependentHandle, object? dependent); | ||||||||
|
|
||||||||
| [MethodImpl(MethodImplOptions.InternalCall)] | ||||||||
| private static extern void InternalFree(IntPtr dependentHandle); | ||||||||
| } | ||||||||
| } | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The details are nice, but this is way too long for a summary, which should be at most one sentence (it's what pops up in IntelliSense for a method). Can you move everything but the first sentence to remarks, editing it appropriately? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this in c463d54, as well as all the other review comments below 🙂