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.

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

[API Proposal]: GCHandle.GetTargetUnsafe() with no handle validation #94113

Closed
Sergio0694 opened this issue Oct 27, 2023 · 5 comments
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices
Milestone

Comments

@Sergio0694
Copy link
Contributor

Background and motivation

GCHandle is often used in scenarios (such as handwritten COM objects, or other interop setups) where callers have complete control over the handles, and nobody else is ever mutating them. The only way, currently, to get a target, is to use GCHandle.Target, which always incurs in a validation check, which is pretty much always just useless branching in these cases.

This proposal is about adding a way to get the handle target with no branches, when callers know no validation is needed.

API Proposal

namespace System.Runtime.InteropServices;

public struct GCHandle
{
+   public readonly object? GetTargetUnsafe();
}

API Usage

// In a constructor (or wherever)
this.handle = GCHandle.Alloc(myObj);

// Later on
object? target = this.handle.GetTargetUnsafe();

Alternative Designs

Alternatively (as @tannergooding also suggested), we could instead just remove the validation check from GCHandle.Target. This would technically be a breaking change, as it would cause invalid handles to throw a NullReferenceException instead of an InvalidOperationException, but it might be considered acceptable in this scenario, since it's a niche API.

The benefit of this approach is that no new API is needed, and everyone "gets faster for free".

Risks

With the new API, no risk.
With the alternate design, people taking a hard dependency on that exception type would break.

@Sergio0694 Sergio0694 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 27, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 27, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 27, 2023
@tannergooding
Copy link
Member

Alternatively (as @tannergooding also suggested), we could instead just remove the validation check from GCHandle.Target. This would technically be a breaking change, as it would cause invalid handles to throw a NullReferenceException instead of an InvalidOperationException, but it might be considered acceptable in this scenario, since it's a niche API.

Notably this suggestion came from the perspective that the current implementation is logically:

ThrowIfInvalid(handle);
handle &= ~(nuint)(1); // strip the pin bit
return InternalGet(handle);

This means that these two calls should be logically the same, but they aren't:

GCHandle.FromIntPtr(0).Target; // InvalidOperation
GCHandle.FromIntPtr(1).Target; // NullReference

We're already handling all kinds of invalid handles and most of them, outside 0, result in a NRE. So it might make sense to simply have 0 also cause a NRE. This removes the branch and makes the API overall more consistent.

@jkotas jkotas added area-System.Runtime.InteropServices and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Oct 27, 2023
@ghost
Copy link

ghost commented Oct 27, 2023

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

GCHandle is often used in scenarios (such as handwritten COM objects, or other interop setups) where callers have complete control over the handles, and nobody else is ever mutating them. The only way, currently, to get a target, is to use GCHandle.Target, which always incurs in a validation check, which is pretty much always just useless branching in these cases.

This proposal is about adding a way to get the handle target with no branches, when callers know no validation is needed.

API Proposal

namespace System.Runtime.InteropServices;

public struct GCHandle
{
+   public readonly object? GetTargetUnsafe();
}

API Usage

// In a constructor (or wherever)
this.handle = GCHandle.Alloc(myObj);

// Later on
object? target = this.handle.GetTargetUnsafe();

Alternative Designs

Alternatively (as @tannergooding also suggested), we could instead just remove the validation check from GCHandle.Target. This would technically be a breaking change, as it would cause invalid handles to throw a NullReferenceException instead of an InvalidOperationException, but it might be considered acceptable in this scenario, since it's a niche API.

The benefit of this approach is that no new API is needed, and everyone "gets faster for free".

Risks

With the new API, no risk.
With the alternate design, people taking a hard dependency on that exception type would break.

Author: Sergio0694
Assignees: -
Labels:

api-suggestion, area-System.Runtime.InteropServices, untriaged, needs-area-label

Milestone: -

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Oct 27, 2023
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the Future milestone Oct 27, 2023
@jkotas
Copy link
Member

jkotas commented Oct 27, 2023

There are other similar inefficiencies in GCHandle:

And there is the cosmetic issue of GCHandle not implementing IDisposable (#54792).

We should consider all of them at the same time. We may want to consider introducing a new type that has all of them fixed.

@Sergio0694
Copy link
Contributor Author

That type looks really nice 👀

If there is interest, what about opening a new proposal for that, linking all these ones, and try to get that to API review?

@Sergio0694
Copy link
Contributor Author

Superseded by #94134, closing this.

@Sergio0694 Sergio0694 closed this as not planned Won't fix, can't repro, duplicate, stale Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices
Projects
Status: No status
Development

No branches or pull requests

4 participants