-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Make DependentHandle public #54246
Make DependentHandle public #54246
Changes from 1 commit
57fe91c
b1f54b5
b331b8f
a96fa3f
16fdf0e
748d88e
247fa5a
66d2ac5
4067ac3
1670339
96cfc91
6a8db56
1601d88
359938b
312851a
0145a76
4925877
1664a95
ca515b6
08df598
4e2b624
4e03297
25b34c2
01f32a3
b3963f2
34e1bcb
9fd1da4
d7146e0
c9c6325
c463d54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,20 +9,25 @@ | |
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> | ||
/// 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 instance. | ||
/// </summary> | ||
/// <remarks> | ||
/// 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> | ||
/// Using 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> | ||
/// <para> | ||
/// 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> | ||
/// </para> | ||
/// <para> | ||
/// The <see cref="IsAllocated"/>, <see cref="Target"/>, <see cref="Dependent"/> and <see cref="TargetAndDependent"/> | ||
/// properties are instead thread-safe, and safe to use if <see cref="Dispose"/> is not concurrently invoked as well. | ||
/// </para> | ||
/// </remarks> | ||
public struct DependentHandle : IDisposable | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add a [DebuggerDisplay(...)] attribute? And/or a [DebuggerTypeProxy(...)]? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
{ | ||
|
@@ -60,13 +65,15 @@ public struct DependentHandle : IDisposable | |
/// <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. | ||
// no need to check for null result: InternalInitialize expected to throw OOM. | ||
_handle = InternalInitialize(target, dependent); | ||
} | ||
|
||
/// <summary> | ||
/// Gets a value indicating whether this handle has been allocated or not. | ||
/// Gets a value indicating whether this instance was constructed with | ||
/// <see cref="DependentHandle(object?, object?)"/> and has not yet been disposed. | ||
/// </summary> | ||
/// <remarks>This property is thread-safe.</remarks> | ||
public bool IsAllocated => (nint)_handle != 0; | ||
|
||
/// <summary> | ||
|
@@ -144,11 +151,12 @@ public object? Dependent | |
/// Gets the values of both <see cref="Target"/> and <see cref="Dependent"/> (if available) as an atomic operation. | ||
/// 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. | ||
/// If <see cref="Target"/> and <see cref="Dependent"/> were used sequentially in this scenario instead, it | ||
/// would 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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thread safety remark? |
||
/// <remarks>This property is thread-safe.</remarks> | ||
public (object? Target, object? Dependent) TargetAndDependent | ||
{ | ||
get | ||
|
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 🙂