[API Proposal]: Introduce SafeHandle.DangerousAddRef()

[adamsitnik]

Background and motivation

From #124375 (comment):

addedRef is always true on return from DangerousAddRef (the argument is left over from thread abort hardeing that we do not care about anymore)

Because of this, the usage often looks like this:

runtime/src/libraries/Common/src/Interop/Windows/Advapi32/SafeHashHandle.cs

Lines 34 to 35 in 8b63bd2

	bool ignored = false;
	_parent.DangerousAddRef(ref ignored);

runtime/src/libraries/Common/src/Interop/Windows/WinHttp/Interop.SafeWinHttpHandle.cs

Lines 39 to 40 in 8b63bd2

	bool ignore = false;
	parentHandle.DangerousAddRef(ref ignore);

API Proposal

namespace System.Runtime.InteropServices;

public abstract partial class SafeHandle
{
+   [EditorBrowsable(EditorBrowsableState.Never)]
    public void DangerousAddRef(ref bool success);
+   public void DangerousAddRef();
}

API Usage

handle.DangerousAddRef();

Risks

DangerousAddRef(ref bool success) is not abstract or virtual, so I can't see any.

[adamsitnik]

cc @jkotas

[dotnet-policy-service]

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

[jkotas]

Because of this, the usage often looks like this:

That's only true if you are doing advanced manual lifetime management that spans multiple methods.

The canonical pattern described in https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.safehandle.dangerousaddref?view=net-10.0#remarks works fairly well with the existing APIs. For example, consider what would the rewrite of

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.NativeMethods.cs

Lines 233 to 251 in 8b63bd2

	bool connectionSuccess = false;
	bool configurationSuccess = false;
	try
	{
	connection.DangerousAddRef(ref connectionSuccess);
	configuration.DangerousAddRef(ref configurationSuccess);
	return ApiTable->ConnectionStart(connection.QuicHandle, configuration.QuicHandle, family, serverName, serverPort);
	}
	finally
	{
	if (connectionSuccess)
	{
	connection.DangerousRelease();
	}
	if (configurationSuccess)
	{
	configuration.DangerousRelease();
	}
	}

look like without the bool returning APIs.

[EditorBrowsable(EditorBrowsableState.Never)]

I do not think we want to hide this API.

I would leave it to interop owners to decide whether this overload is worth adding.

[stephentoub]

For reference, we have it internally for use in corelib:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/SafeHandle.cs

Lines 180 to 185 in ef560fa

	// Used by internal callers to avoid declaring a bool to pass by ref
	internal void DangerousAddRef()
	{
	bool success = false;
	DangerousAddRef(ref success);
	}

where it has a few but only a few callers. Even with that there are more callers in corelib that use the public API than there are this one. That's generally because either the DangerousAddRef call is made in the middle rather than beginning of the try, or because the ref is to a field/parameter rather than a local (there are a few though that could be switched).

[DrkWzrd]

Off-topic question, why both methods are prefixed with "Dangerous"? I mean, I think I know what Dangerous means in .net context, but in this specific case I don't get it.

[AaronRobinsonMSFT]

@adamsitnik I think we need a more compelling argument for this. The pattern mentioned in #125265 (comment) is really how this API should be used and it is fairly rare where people outside of SPCL should be using it.

Have you observed a performance impact for this change or is this simply an annoyance?

Off-topic question, why both methods are prefixed with "Dangerous"? I mean, I think I know what Dangerous means in .net context, but in this specific case I don't get it.

@DrkWzrd The API is considered "Dangerous" because it exposes the lifetime of the handle, or handle itself, outside of the SafeHandle infrastructure. Once you manually increment the reference count and extract the handle (that is, DangerousGetHandle()), the runtime can no longer guarantee the handle won't be used after being freed. These APIs are meant for bypassing the "prevent release during use" mechanism that SafeHandle was designed to provide. It's on developer to ensure a matching DangerousRelease() is called, even in exceptional cases, or else the handle may leak or be double-freed.