Make DependentHandle public #54246

Sergio0694 · 2021-06-15T21:32:06Z

Closes #19459

dotnet-issue-labeler · 2021-06-15T21:32:10Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

jkotas · 2021-06-15T22:27:12Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+        /// </summary>
+        public object? Target
+        {
+            get => nGetPrimary(_handle);


Does this need null checks like GCHandle? Otherwise, we would crash and burn with hard to understand crash when somebody calls it on nulled out handle.

I just left that as the current implementation here:

runtime/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/DependentHandle.cs

Line 43 in 03b1b38

public object? GetPrimary() => nGetPrimary(_handle);

CoreCLR throws an exception immediately if someone does that, which I guess would be fine? As in, we avoid one extra conditional branch here just to throw maybe a different exception type, but we'd still just throw. Considering the type is very niche and meant for advanced and perf scenarios, it could be fine to leave it as it was? 🤔

The runtime doesn't check this in RELEASE (there's just an _ASSERTE):

runtime/src/coreclr/vm/comdependenthandle.cpp

Lines 57 to 64 in d0ca735

FCIMPL1(Object*, DependentHandle::nGetPrimary, OBJECTHANDLE handle)

{

FCALL_CONTRACT;

FCUnique(0x54);

_ASSERTE(handle != NULL);

return OBJECTREFToObject(ObjectFromHandle(handle));

}

FCIMPLEND

And indeed (on CoreCLR), trying this out just throw an ExecutionEngineException. I'm just thinking that the important bit is getting immediate feedback if this happens (which we do), so considering the scenario we can avoid the overhead to explicitly check this and throw something else? Also because I wouldn't want making this type public to indirectly cause regressions in ConditionalWeakTable<,> either, given that that type uses this heavily. 🙂

We treat ExecutionEngineException as a internal bug in the runtime. It is what the documentation says, it is what automatic classification of crashes does, ... .

I agree that this type is performance critical, but it is not that performance critical. Each method is still a call at least.

Right, yeah that's fair 🙂
The Mono implementation currently checks as well, and it throws a NotSupportedException (which I'm not sure is the right exception type though). What about adding a check to the CoreCLR version to throw InvalidOperationException, and then also aligning the Mono version to throw the same type? Otherwise, let me know what you'd prefer!

Awesome! 😄
Did the changes in 4f602b995ba8f5531136a9370a4af9819e85c194 for both CoreCLR and Mono.

I guess the only remaining questions with respect to usage safety then are:

Right now Dispose is not thread-safe. Should we use Interlocked.Exchange to make it so?

Even if we made Dispose thread-safe, the type itself would still not be thread-safe. Eg. if you concurrently called Dispose and Primary, you could end up trying to retrieve a primary (ie. calling nGetPrimary) with a handle that has concurrently been disposed already by another thread. Should we document this?

The type is not safe at all if someone makes multiple copies (as we discussed in the linked issue with @bartonjs). Do we document this and accept the undefined behavior if someone uses it incorrectly and does weird stuff with multiple instances?

Additional question: would you like me to also add some internal unsafe versions of those APIs (without the handle null check) so that ConditionalWeakTable<,> would be able to keep using them and not incur in any additional overhead due to these changes? That type had been specifically written from the start to ensure those checks were respected anyway (given that they were not there before), so I'm thinking it might make sense to skip the extra checks in that case? Let me know if you think this would be worth it or not necessary, and I'll make the changes if needed 🙂

Right now Dispose is not thread-safe. Should we use Interlocked.Exchange to make it so?

I do not have an opinion on this one. What do you think?

Additional question: would you like me to also add some internal unsafe versions of those APIs (without the handle null check

Yes, that's probably a good idea - to avoid dealing with the micro-benchmark regressions.

"I do not have an opinion on this one. What do you think?"

Mmh I'm thinking in this case it might be best to just leave it as is and properly document the whole type as not being thread-safe? Because as mentioned before even if we made Dispose() itself thread-safe, the whole type itself would still not be thread-safe, which I guess could potentially be even more confusing for consumers. As in, I wouldn't want some consumers to incorrectly assume that just because Dispose() itself is thread-safe, the whole type is as well, potentially leading them towards hard to track bugs. Also, I would expect most people to dispose handles from a finalizer, where thread-safety wouldn't generally be a concern. Does this make sense? 🙂

"Yes, that's probably a good idea - to avoid dealing with the micro-benchmark regressions."

Great, will do that then! 😄

EDIT: done in 603b3cd.

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

src/libraries/System.Runtime/ref/System.Runtime.cs

stephentoub · 2021-06-16T14:49:53Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+                IntPtr handle = _handle;
+                _handle = IntPtr.Zero;
+                nFree(handle);


I know there's been some discussion on this, but...

GCHandle.Free does use an interlocked. Do we know why? Are there uses of GCHandle that end up depending on that? And if yes, do such uses not apply to DependentHandle as well?

I'm fine without the interlocked if we believe it's really unnecessary. (It's obviously there for a different use case than free'ing two copies of the same handle, presumably something where a field of some object stores a GCHandle and two threads might attempt to Free it concurrently for some reason.)

Not entirely sure why does GCHandle use an interlocked exchange, considering that it still has the same issue with respect to value copies anyway. We've been talking about this aspect too with @jkotas (see comment above), and also with @tannergooding on Discord. For instance, Tanner mentioned how MemoryHandle that was added recently also implements IDisposable, but has the same issue with respect to concurrent/copied frees, and so that it would probably be fine to just not use an interlocked here either. My argument for not using interlocked here was also that it could potentially be confusing for consumers given that the type itself would still not be thread-safe anyway (eg. calling Dispose together with any other API could still result in a double free), so I was thinking that just not making any individual method thread-safe and clearly documenting the whole type as not being thread-safe would be easier to understand for people using DependentHandle as well. Thoughts? 🙂

I'd probably defer to @Maoni0 / @jkotas on if it being interlocked is actually important.

It would still be impactful if it were the field of a class and was being used/passed byref.

If it helps, Interlocked.CompareExchange was added to GCHandle.Free as part this change:

change 910279 edit on 2004/08/31 15:02:42 New MDA DCR - detects when bad GCHandles are retrieved.

This changed introduced a parallel tracking structure for live GC handles that was automatically enabled when running under debugger. It was important for this parallel tracking structure to stay in sync with the actual GC handles and to be robust against incorrect GCHandle use (the point of the MDA was to catch incorrect uses of GCHandles). InterlockedCompare helped to achieve it.

The MDA is long gone, but the InterlockedCompareExchange stayed.

If we keep the impl as is, we should explicitly say in the docs that the Target and Dependent properties are thread safe and that Dispose is not thread safe.

Added some notes both on the type and on the individual APIs in 9c23a947ce5da21fb6169b6dae16a38f1412e8e9 🙂

stephentoub · 2021-06-16T14:51:57Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+    /// that object having a field or property (or some other strong reference) to a dependent object instance B.
+    /// </para>
+    /// </summary>
+    public struct DependentHandle : IDisposable


Should we add a [DebuggerDisplay(...)] attribute? And/or a [DebuggerTypeProxy(...)]?

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. [DebuggerDisplay] could add more info, but wouldn't a [DebuggerTypeProxy] with the same IsAllocated, Target and Dependent properties offer the same info as what VS would already do on its own? Or is that to avoid throwing InvalidOperationExceptions in the debugger if the handle is not allocated, or something else? Thanks! 🙂

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? 🤔

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.

stephentoub · 2021-06-16T14:55:04Z

src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+    //
+    public struct DependentHandle : IDisposable
+    {
+        private Ephemeron[] data;


All of the access to this appears to be guarded by null checks (unless I've missed a code path that doesn't). Should this be ?? Then the null! in Dispose could lose the !.

Done in f64d922a6000f6b9379048f8afaa8ec0b2a8d71c 👍

stephentoub · 2021-06-16T14:55:11Z

src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+    //
+    public struct DependentHandle : IDisposable
+    {
+        private Ephemeron[] data;


Fixed in f64d922a6000f6b9379048f8afaa8ec0b2a8d71c 🙂

stephentoub · 2021-06-16T14:57:11Z

src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+    internal struct Ephemeron
+    {
+        public object? key;
+        public object? value;


Does the mono runtime care about these exact names? If not, Key and Value.

I don't think it does, as far as I can tell 🤔
The Ephemeron type is defined here:

runtime/src/mono/mono/metadata/sgen-mono.c

Lines 561 to 564 in 155543f

typedef struct {

GCObject *key;

GCObject *value;

} Ephemeron;

So I think as long as the layout matches, the field names on the C# side won't matter.
I'll change this and the field name as well like you mentioned in the comment above 👍

EDIT: done in 50867efe8cef2f50638082e1ce1d93667656229e.

stephentoub · 2021-06-16T14:59:25Z

src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+            if (data[0].key == GC.EPHEMERON_TOMBSTONE)
+            {
+                return default;
+            }
+
+            return (data[0].key, data[0].value);


Is it possible data[0] could transition to GC.EPHEMERON_TOMBSTONE between the time it's checked above and here? If yes, it should probably be more like:

Ephemeron e = data[0]; return e.key != GC.EPHEMERON_TOMBSTONE ? (e.key, e.value) : default;

I have to say that I asked myself the same exact question as well while refactoring 😅
I left it as it was for now just to avoid making the PR more complex than intended.
This is how the type is currently implemented in Mono:

runtime/src/mono/System.Private.CoreLib/src/System/Runtime/CompilerServices/DependentHandle.cs

Lines 45 to 51 in ea69731

if (data[0].key == GC.EPHEMERON_TOMBSTONE)

{

secondary = null;

return null;

}

secondary = data[0].value;

return data[0].key;

Which to me does look like you could end up reading a tombstone and a null if the GC stopped everything right after the branch and collected the key and value. But then again I'd have expected such an issue to have come up already given that ConditionalWeakTable<,> is heavily used already? 🤔

Let me know if you'd still like me to change these implementations on Mono anyway!

Yes, it would be nice to fix.

I'd have expected such an issue to have come up already given that ConditionalWeakTable<,> is heavily used already?

It is a race condition. Race conditions like this can linger in the product for a long time. The GC has to happen at the right spot, there needs to be a follow up code that leads to a crash, and we must be lucky to capture the crashdump and connect the dots to this bug. Also, Mono is typically used in less stressful workloads vs. CoreCLR so it makes it even less likely for this bug to be found and fixed.

I see, thanks! I got that it needed some very specific conditions to happen and to also be properly detected, I guess I just thought Mono was being used so much considering all the various platforms it supports, and CWT<,> being pretty common lower in the stack, that it'd have caused more noticeable issues already 😄

Fixed in 9f7b212d39f6d9b39db764a3032f0a4ff7dddf38 for all APIs.

CWT<,> being pretty common lower in the stack

In dotnet/runtime, CWT<,> is only used on paths that are rarely used or generally slow for other reasons. It is not stressed much by the lower levels of the stack.

Right, I was thinking that given that eg. ArrayPool<T>.Shared uses it too now (here) and that type is getting relatively used more and more frequently, that could influence that, but as you said that's only really used in slower paths, so probably not under all that actual stress after all. Thanks for the additional info on that. I guess really I'm just always surprised whenever bugs like this are discovered in the runtime - of course no software is actually bug-free, but one often tends to think of the runtime as being almost infallible 😄

stephentoub · 2021-06-16T15:00:02Z

src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+                return null;
+            }
+
+            if (data[0].key == GC.EPHEMERON_TOMBSTONE)


Same question

stephentoub · 2021-06-16T15:00:43Z

src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+        {
+            // Getting the secondary object is more expensive than getting the first so
+            // we provide a separate primary-only accessor for those times we only want the
+            // primary.


Is this true for the mono implementation as well? That seems... odd... given how it's structured.

I agree, don't think that comment is accurate for Mono. It looks like the Mono implementation of DependentHandle had just copied that comment from the CoreCLR reference source and forgot to remove it or to at least update according to their own architecture. Retrieving either the key or the value on Mono does indeed seem to be equally expensive as you said. I have removed it in b6d306e41d1ab4f0e8692b2c16defab1314cacad 🙂

src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs

stephentoub · 2021-06-16T15:04:50Z

src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs

+            Assert.Throws<InvalidOperationException>(() =>
+            {
+                DependentHandle handle = default;
+                handle.Dependent = new();
+            });


Ditto:

Suggested change

Assert.Throws<InvalidOperationException>(() =>

{

DependentHandle handle = default;

handle.Dependent = new();

});

Assert.Throws<InvalidOperationException>(() => default(DependentHandle).Dependent = new());

src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs

stephentoub · 2021-06-16T15:07:56Z

src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs

+namespace System.Runtime.Tests
+{
+    // NOTE: DependentHandle is already heavily tested indirectly through ConditionalWeakTable<,>.
+    // This class contains some specific tests for APIs that are only relevant when used directly.


I think we should still have a reasonable set of tests for DependentHandle directly covering its basic functionality. Its use in ConditionalWeakTable is an implementation detail.

Added some tests for the available public APIs (getting/setting target/dependent, and liveness tests) in a81d5f8, happy to add more if there's additional specific scenarios you think we should also check in particular! 🙂

stephentoub · 2021-06-16T15:09:16Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+        /// </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()


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

?

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 Math APIs recently added). Jan said either was fine for him, so I just went with the method in this case 😄

given that the method returned a tuple, it felt much more appropriate to him for it to be a method instead

Why? We have many properties that return structs with multiple properties. What makes a ValueTuple special?

Can't talk for Jeremy specifically, what he said on stream was:

"Having a property that returns a tuple feels weird, it feels more method-y"

"[...] for reasons that I can't articulate, maybe artistic style, this feels more like a method than a property"

"[...] TargetAndDependent should be a method, not a property. It feels really weird as a property".

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 Target and Dependent. As in, we have a property for each entity, and if you want to get both of them together, we then have a method that retrieves both entities and returns them. Having both a property for each entity and also a property for both looks really weird to me. And also the fact that we've just never used a value-tuple in a property before, while we have in methods. I just really agree with Jeremy that a method just feels right in this case, is all 😄

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 GC.KeepAlive too, so it is there just for convenience.

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 GC.KeepAlive vs. recomending to use the tuple returning method.

Yup, was just going through the VM trying to understand exactly how that hack actually worked 😄
I wasn't aware that both GCHandle and DependentHandle still used the same OBJECTHANDLE struct in the VM (GCHandleManager::CreateGlobalHandleOfType and GCHandleStore::CreateDependentHandle). After going through the code and having a read at this paragraph I think I get the general gist of that seemingly magic trick, will go try to make that change you suggested. One never stops learning cool stuff by contributing to the runtime! 🙌

@jkotas After checking it looks like that with Andy's recent jump threading changes (#46257), the JIT can now correctly skip the second null check if you get Target and Dependent sequentially (given that handle just gets read to a register anyway), which after the optimization to nGetPrimary would've been the only remaining advantage for GetTargetAndDependent(). I think with that then the codegen for both things should be pretty much the same, so if you prefer to remove that API I guess it should be safe to do so now without having performance overhead for public consumers? Happy to add those remarks about GC.KeepAlive as well 🙂

Yes, I would vote to remove the tuple returning APIs and just document the race conditions to be careful about.

Removed in 9c23a947ce5da21fb6169b6dae16a38f1412e8e9 and added the documentation about GC.KeepAlive 👍

I still see it in the PR. Did you intend to remove it?

jkotas · 2021-06-16T16:47:56Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+        private static extern IntPtr nInitialize(object? primary, object? secondary);
+
+        [MethodImpl(MethodImplOptions.InternalCall)]
+        private static extern object? nGetPrimary(IntPtr dependentHandle);


You can optimize nGetPrimary for CoreCLR using the same technique as what is used for GCHandle.Target.

Once you do that, I think we can change nGetPrimaryAndSecondary to just nGetSecondary.

Added the optimization in bc9ee2e65c06b6766faaeef3f403bb8aa794118a, and also left a detailed comment for future reference 😄

Looking into changing that FCall now!

EDIT: done in 3405e67.

src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

Sergio0694 · 2021-06-21T20:54:01Z

Alright, I think the PR is in a good spot now - marking is as ready for review 😄

I've switched Target to be readonly and added the new new StopTracking() API as mentioned above, and made some tweaks to the FCall on CoreCLR to account for it (with the input being a constant, we can skip it and hardcode it directly now). I've also updated the logic on Mono so that it would be consistent with CoreCLR (given that DependentHandle is now public). Added some tests for all these scenarios involving StopTracking() as well, and double checked that ConditionalWeakTable<K, V> is now working fine again with the updated APIs. The current CI failures seem unrelated from these changes from what I can tell 🙂

stephentoub · 2021-06-21T21:00:36Z

the new new StopTracking() API

This would need to go through API review (I personally don't like the name, as there's nothing else anywhere in the surface area that suggests something is being "tracked" or that such tracking was "started".) If it's there in support of ConditionalWeakTable, I suggest making it internal for now so as to unblock the rest of the PR... unless we think it's important for anyone to be able to use the type, in which case we should wait on it.

Sergio0694 · 2021-06-21T21:11:09Z

@stephentoub Absolutely happy to change the name of course, really just used that there just to at least make the actual changes in the meantime and validate that the whole thing was working as expected (consider it a placeholder, if you will 😄 ). I was expecting Jan to eventually chime in anyway and make a final decision on the whole PR and the new API tweaks (unless the plan was to go through API review again, but I think @tannergooding mentioned that might not be necessary?).

"If it's there in support of ConditionalWeakTable, I suggest making it internal for now so as to unblock the rest of the PR... unless we think it's important for anyone to be able to use the type, in which case we should wait on it."

One of the main reasons also discussed during API review for making this type public was to enable consumers to implement custom conditional weak tables. For instance the whole reason why I've pushed so much for this whole proposal is that I plan to use this in the next release of the MVVM Toolkit (where I'll also add the .NET 6 target), where having access to DependentHandle will allow us to achieve 0-allocation broadcasting in our messenger type, specifically using a custom conditional weak table with a value-struct enumerator (I've left some more details in my comment here). The reason why we ended up with his proposal was because Jan mentioned it'd have been more useful for the whole ecosystem to just expose DependentHandle rather than just add a value-struct enumerator to ConditionalWeakTable<K, V> in particular (which was my original proposal here). As such though, it'd be absolutely crucial for us to have the public version of DependentHandle to expose all the APIs that ConditionalWeakTable<K, V> relies on, otherwise it would kinda defeat the whole purpose for us 🙂

jkotas · 2021-06-21T22:06:03Z

One of the main reasons also discussed during API review for making this type public was to enable consumers to implement custom conditional weak tables.

Note that you can still do that, even without settable Target and the API to get both Target and Dependent atomically. It just going be a bit less performant than the built-in ConditionalWeakTable.

Sergio0694 · 2021-06-21T22:22:56Z

Note that you can still do that, even without settable Target and the API to get both Target and Dependent atomically.

Do you mean by taking a lock for readers as well, and having the remove method dispose, and then check that? I mean I can see how that would work but I was hoping to keep the performance difference to a minimum, considering that the type is pretty niche anyway. The public API are already taking a small performance hit due to the additional allocation check that the internal versions don't have 😟

Also I'm not going to be the only one using the type and I would assume other consumers would be happy to have all those APIs available as well, especially the ones that are currently hacking DependentHandle through private reflection today and using them. This is just my 2 cents here, but what I'm trying to say is that I'm not convinced that limiting the public API surface would be the best decision here, considering how incredibly niche and perf oriented the type itself was meant to be from the beginning, and the fact that those APIs are already there, they're still pretty simple and don't constitute much more code to maintain at all (especially given that we'd need to keep them anyway as they're used internally), and they've been proven valuable already through ConditionalWeakTable. Personally I would really suggest to expose them publicly as well, though of course with whatever signature/name the team prefers 🙂

jkotas · 2021-06-21T22:40:58Z

My worry is that we will tie our hands for future by exposing the special operations that the conditional weak table uses today and that exploit the implementation details. Ephmeron-like types in other runtimes do not have these special operations.

Sergio0694 · 2021-06-21T22:54:47Z

I see, yeah that's a totally fair point. While we're talking about this, here's an idea then, to possibly help with the issue about future flexibility you mentioned. This was actually one of the ideas I had before bringing up StopTracking. Consider the following:

public struct DependentHandle
{
    public DependentHandle(object? target, object? dependent);

    public object? Target { get; set; }
    
    // Other APIs...
}

In this scenario, we'd keep the setter, but we'd document it to only allow null values to be set. This would mean that:

The current functionality for StopTracking() would be preserved: just set to null.
We would still solve the potential GC corruption issues you mentioned the other day, as we'd forbid swapping targets.
We'd have flexibility to possibly expand this in the future (say, if the GC added support for changing targets), as we could then update the docs and remove the check, without having to make any breaking API changes nor add new APIs.
This would also already work for Mono, which supported setting a different target already. For consistency with CoreCLR we would just keep the check for now so that both implementations would behave the same (as they're doing now).

So we'd keep flexibility while at the same time allowing CWT to still keep the same implementation, and for external consumers (both in case they wanted to just copy CWT and do some tweaks, or needed the APIs for some other reason) to still have access to all the fundamental building blocks to make all of that work without requiring refactoring, changes, or more overhead.

Thoughts? 🙂

EDIT: just realized, this would actually also be the same API surface approved by API review 😄
(with the only difference being the extra caveat on values passed to the setter, only allowing null)

Sergio0694 · 2021-06-22T17:13:50Z

Alright, I've refactored the API surface in c9c6325 to match exactly the one that was formally approved during API review last week, with the only difference being the presence of IDisposable which was suggested by @jkotas in the previous conversation.

Here's the current API surface:

public struct DependentHandle : IDisposable
{
    public DependentHandle(object? target, object? dependent);
    public bool IsAllocated { get; }
    public object? Target { get; set; }
    public object? Dependent { get; set; }
    public (object? Target, object? Dependent) TargetAndDependent { get; }
    public void Dispose();
}

Specifically, to address the previous points that were raised:

TargetAndDependent is a property, as was requested by @stephentoub and @tannergooding.
The setter for Target only accepts null values to guard against GC holes, which was an issue brought up by Jan. This is documented in the API and allows the implementation to possibly be expanded in the future if CoreCLR gained support for updating the target to another live instance. Right now Mono follows the same behavior as CoreCLR, for consistency.
No other weird/new APIs are present anymore, this is exactly as approved during API review (minor notes above) 😄

jkotas

It looks good to me. Thanks

@stephentoub @tannergooding @Maoni0 Could you please take a look as well?

stephentoub · 2021-06-23T01:39:20Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

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


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!

Fixed this in c463d54, as well as all the other review comments below 🙂

stephentoub · 2021-06-23T01:40:58Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+        /// </summary>
+        /// <exception cref="InvalidOperationException">
+        /// Thrown if <see cref="IsAllocated"/> is <see langword="false"/> or if the input value is not <see langword="null"/>.</exception>
+        /// <remarks>This property is thread-safe.</remarks>


IsAllocated is missing a similar thread-safety remark.

stephentoub · 2021-06-23T01:41:49Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

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


Thread safety remark?

stephentoub · 2021-06-23T01:48:45Z

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs

+        }
+
+        /// <summary>
+        /// Gets a value indicating whether this handle has been allocated or not.


Suggested change

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

Sergio0694 · 2021-06-23T09:28:34Z

Thank you so much @jkotas and @stephentoub for your time reviewing this and all your feedbacks and suggestions!
It was super fun working on this PR and it also let me learn some new things along the way! 😄🙌

danmoseley · 2021-06-23T14:44:26Z

@Sergio0694 we appreciate your work! Perhaps after this is merged you can spot another issue that would be your next interesting project?

Sergio0694 · 2021-06-23T16:12:43Z

Thanks @danmoseley! And yeah for sure, would love to contribute some more! 🙌

I'm already thinkering with #54611, might be a bit trickier than expected as Jan and Stephen pointed out, but we'll see 😄

Sergio0694 · 2021-06-26T22:58:59Z

This is amazing, we made it!! 🎉🎉🎉
Thanks again to the whole team for making this possible!

jkotas reviewed Jun 15, 2021

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs Outdated Show resolved Hide resolved

Sergio0694 force-pushed the sergio0694/public-dependent-handle branch from 0ca0dc5 to 2a8760f Compare June 15, 2021 23:21

shmuelie reviewed Jun 15, 2021

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs Show resolved Hide resolved

jkotas reviewed Jun 16, 2021

View reviewed changes

src/libraries/System.Runtime/ref/System.Runtime.cs Show resolved Hide resolved

Sergio0694 force-pushed the sergio0694/public-dependent-handle branch 3 times, most recently from 3dc0cab to 043ecb3 Compare June 16, 2021 10:04

stephentoub reviewed Jun 16, 2021

View reviewed changes

src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs Show resolved Hide resolved

stephentoub reviewed Jun 16, 2021

View reviewed changes

src/libraries/System.Runtime/tests/System/Runtime/DependentHandleTests.cs Show resolved Hide resolved

stephentoub reviewed Jun 16, 2021

View reviewed changes

Sergio0694 force-pushed the sergio0694/public-dependent-handle branch from 625bfb8 to ac7c412 Compare June 16, 2021 16:01

jkotas reviewed Jun 16, 2021

View reviewed changes

src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 16, 2021

View reviewed changes

src/mono/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs Outdated Show resolved Hide resolved

tannergooding reviewed Jun 16, 2021

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 16, 2021

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 16, 2021

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs Outdated Show resolved Hide resolved

Sergio0694 force-pushed the sergio0694/public-dependent-handle branch from 117802f to 641b405 Compare June 16, 2021 18:51

Sergio0694 marked this pull request as ready for review June 21, 2021 20:53

Sergio0694 requested a review from marek-safar as a code owner June 21, 2021 20:53

Update API surface to match approved specs from API review

c9c6325

Sergio0694 force-pushed the sergio0694/public-dependent-handle branch from e545245 to c9c6325 Compare June 22, 2021 17:41

jkotas approved these changes Jun 22, 2021

View reviewed changes

stephentoub reviewed Jun 23, 2021

View reviewed changes

stephentoub approved these changes Jun 23, 2021

View reviewed changes

Update DependentHandle XML docs

c463d54

tannergooding approved these changes Jun 23, 2021

View reviewed changes

karelz mentioned this pull request Jun 24, 2021

Connect_DualMode_DnsConnect_RetrievedEndPoints_Success - Assert ::1 #54677

Closed

jkotas closed this Jun 26, 2021

jkotas reopened this Jun 26, 2021

stephentoub merged commit 02f70d0 into dotnet:main Jun 26, 2021

Sergio0694 deleted the sergio0694/public-dependent-handle branch June 26, 2021 22:57

ghost locked as resolved and limited conversation to collaborators Jul 26, 2021

	FCIMPL1(Object*, DependentHandle::nGetPrimary, OBJECTHANDLE handle)
	{
	FCALL_CONTRACT;
	FCUnique(0x54);
	_ASSERTE(handle != NULL);
	return OBJECTREFToObject(ObjectFromHandle(handle));
	}
	FCIMPLEND

	if (data[0].key == GC.EPHEMERON_TOMBSTONE)
	{
	secondary = null;
	return null;
	}
	secondary = data[0].value;
	return data[0].key;

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

Make DependentHandle public #54246

Make DependentHandle public #54246

Conversation

Sergio0694 commented Jun 15, 2021 • edited Loading

Closes #19459

dotnet-issue-labeler bot commented Jun 15, 2021

Choose a reason for hiding this comment

Sergio0694 Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

jkotas Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

stephentoub Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

stephentoub Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Sergio0694 commented Jun 21, 2021

stephentoub commented Jun 21, 2021

Sergio0694 commented Jun 21, 2021

jkotas commented Jun 21, 2021

Sergio0694 commented Jun 21, 2021

jkotas commented Jun 21, 2021

Sergio0694 commented Jun 21, 2021 • edited Loading

Sergio0694 commented Jun 22, 2021 • edited Loading

jkotas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 commented Jun 15, 2021 •

edited

Loading

Sergio0694 Jun 15, 2021 •

edited

Loading

jkotas Jun 15, 2021 •

edited

Loading

Sergio0694 Jun 15, 2021 •

edited

Loading

Sergio0694 Jun 16, 2021 •

edited

Loading

stephentoub Jun 16, 2021 •

edited

Loading

tannergooding Jun 16, 2021 •

edited

Loading

Sergio0694 Jun 16, 2021 •

edited

Loading

stephentoub Jun 17, 2021 •

edited

Loading

stephentoub Jun 16, 2021 •

edited

Loading

Sergio0694 Jun 16, 2021 •

edited

Loading

Sergio0694 Jun 16, 2021 •

edited

Loading

Sergio0694 Jun 16, 2021 •

edited

Loading

Sergio0694 Jun 17, 2021 •

edited

Loading

Sergio0694 Jun 16, 2021 •

edited

Loading

Sergio0694 commented Jun 21, 2021 •

edited

Loading

Sergio0694 commented Jun 22, 2021 •

edited

Loading