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]: Marshaller type to support SafeHandle in source-generated marshalling without hard-coding in the generator #74035

Closed
jkoritzinsky opened this issue Aug 16, 2022 · 2 comments · Fixed by #85419
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Milestone

Comments

@jkoritzinsky
Copy link
Member

Background and motivation

Today, we support marshalling System.Runtime.InteropServices.SafeHandle-derived types intrinsically in the interop source generators. This is one of the only types that we do this for since we moved string marshalling out of the generator during the .NET 7 development cycle. Additionally, SafeHandle marshalling is extremely complex, much more than any other built-in type.

As a result, we'd like to introduce a marshaller type so we can move the interop source generators to use this type instead of emitting the code directly.

This change will make the interop code more serviceable, since we can't service source-generated code.

API Proposal

namespace System.Runtime.InteropServices.Marshalling;

[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.ManagedToUnmanagedIn, typeof(ManagedToUnmanagedIn))]
[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.ManagedToUnmanagedRef, typeof(ManagedToUnmanagedRef))]
[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.ManagedToUnmanagedOut, typeof(ManagedToUnmanagedOut))]
public static class SafeHandleMarshaller<T>
    where T : System.Runtime.InteropServices.SafeHandle
{
    public struct ManagedToUnmanagedIn
    {
        public void FromManaged(T handle);

        public IntPtr ToUnmanaged();

        public void Free();
    }

    public struct ManagedToUnmanagedRef
    {
        public ManagedToUnmanagedRef();

        public void FromManaged(T handle);

        public IntPtr ToUnmanaged();

        public void FromUnmanaged(IntPtr value);

        public T ToManagedGuaranteed();

        public void OnInvoked();

        public void Free();
    }

    public struct ManagedToUnmanagedOut
    {
        public ManagedToUnmanagedOut();

        public void FromUnmanaged(IntPtr value);

        public T ToManaged();

        public void OnInvoked();

        public void Free();
    }
}

API Usage

// Marshaller will be implicitly used
[LibraryImport("foo")]
public void Method(Microsoft.Win32.SafeFileHandle handle);
[LibraryImport("foo")]
public void Method2([MarshalUsing(typeof(SafeHandleMarshaller<Microsoft.Win32.SafeFileHandle>))] Microsoft.Win32.SafeFileHandle handle);

Alternative Designs

We considered adding a new() constraint to the generic parameter to enforce our requirements around constructing new instances of the SafeHandle-derived type for the ref and out marshallers; however, this constraint would cause breaking changes for APIs that take non-concrete SafeHandle types as by-value or by-ref in parameters.

Risks

No response

@jkoritzinsky jkoritzinsky added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices labels Aug 16, 2022
@jkoritzinsky jkoritzinsky added this to the 8.0.0 milestone Aug 16, 2022
@ghost
Copy link

ghost commented Aug 16, 2022

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

Today, we support marshalling System.Runtime.InteropServices.SafeHandle-derived types intrinsically in the interop source generators. This is one of the only types that we do this for since we moved string marshalling out of the generator during the .NET 7 development cycle. Additionally, SafeHandle marshalling is extremely complex, much more than any other built-in type.

As a result, we'd like to introduce a marshaller type so we can move the interop source generators to use this type instead of emitting the code directly.

This change will make the interop code more serviceable, since we can't service source-generated code.

API Proposal

namespace System.Runtime.InteropServices.Marshalling;

[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.ManagedToUnmanagedIn, typeof(ManagedToUnmanagedIn))]
[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.ManagedToUnmanagedRef, typeof(ManagedToUnmanagedRef))]
[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.ManagedToUnmanagedOut, typeof(ManagedToUnmanagedOut))]
public static class SafeHandleMarshaller<T>
    where T : System.Runtime.InteropServices.SafeHandle
{
    public struct ManagedToUnmanagedIn
    {
        public void FromManaged(T handle);

        public IntPtr ToUnmanaged();

        public void Free();
    }

    public struct ManagedToUnmanagedRef
    {
        public ManagedToUnmanagedRef();

        public void FromManaged(T handle);

        public IntPtr ToUnmanaged();

        public void FromUnmanaged(IntPtr value);

        public T ToManagedGuaranteed();

        public void OnInvoked();

        public void Free();
    }

    public struct ManagedToUnmanagedOut
    {
        public ManagedToUnmanagedOut();

        public void FromUnmanaged(IntPtr value);

        public T ToManaged();

        public void OnInvoked();

        public void Free();
    }
}

API Usage

// Marshaller will be implicitly used
[LibraryImport("foo")]
public void Method(Microsoft.Win32.SafeFileHandle handle);
[LibraryImport("foo")]
public void Method2([MarshalUsing(typeof(SafeHandleMarshaller<Microsoft.Win32.SafeFileHandle>))] Microsoft.Win32.SafeFileHandle handle);

Alternative Designs

We considered adding a new() constraint to the generic parameter to enforce our requirements around constructing new instances of the SafeHandle-derived type for the ref and out marshallers; however, this constraint would cause breaking changes for APIs that take non-concrete SafeHandle types as by-value or by-ref in parameters.

Risks

No response

Author: jkoritzinsky
Assignees: -
Labels:

api-suggestion, area-System.Runtime.InteropServices

Milestone: 8.0.0

@AaronRobinsonMSFT AaronRobinsonMSFT added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Aug 17, 2022
@bartonjs
Copy link
Member

bartonjs commented Sep 20, 2022

Video

  • Looks good as proposed.
namespace System.Runtime.InteropServices.Marshalling;

[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.ManagedToUnmanagedIn, typeof(ManagedToUnmanagedIn))]
[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.ManagedToUnmanagedRef, typeof(ManagedToUnmanagedRef))]
[CustomMarshaller(typeof(CustomMarshallerAttribute.GenericPlaceholder), MarshalMode.ManagedToUnmanagedOut, typeof(ManagedToUnmanagedOut))]
public static class SafeHandleMarshaller<T>
    where T : System.Runtime.InteropServices.SafeHandle
{
    public struct ManagedToUnmanagedIn
    {
        public void FromManaged(T handle);

        public IntPtr ToUnmanaged();

        public void Free();
    }

    public struct ManagedToUnmanagedRef
    {
        public ManagedToUnmanagedRef();

        public void FromManaged(T handle);

        public IntPtr ToUnmanaged();

        public void FromUnmanaged(IntPtr value);

        public T ToManagedGuaranteed();

        public void OnInvoked();

        public void Free();
    }

    public struct ManagedToUnmanagedOut
    {
        public ManagedToUnmanagedOut();

        public void FromUnmanaged(IntPtr value);

        public T ToManaged();

        public void OnInvoked();

        public void Free();
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 20, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 26, 2023
jkoritzinsky added a commit that referenced this issue May 3, 2023
…ling support. (#85419)

Fixes #74035

We can't remove the built-in marshalling support from the generator yet, but once the out-of-band packages we ship don't support .NET 6. we can remove the built-in support that emits the marshalling code in the stub. I believe the .NET 9 packages won't support .NET 6, so once we snap for .NET 9 and update how we ship the packages, we can clean this up.

This PR also adds a requested feature to the SafeHandle marshaller: If the call to native code fails, we'll call Dispose() on the pre-allocated handle to avoid leaking it to the finalizer queue.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 3, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Projects
None yet
3 participants