Skip to content

Conversation

@DoctorKrolic
Copy link
Contributor

Before this change null checking of safe handles inside friendly overloads was hapenning inside main method logic, e.g.:

internal static unsafe winmdroot.Foundation.WAIT_EVENT SignalObjectAndWait(SafeHandle hObjectToSignal, SafeHandle hObjectToWaitOn, uint dwMilliseconds, winmdroot.Foundation.BOOL bAlertable)
{
	bool hObjectToSignalAddRef = false;
	bool hObjectToWaitOnAddRef = false;
	try
	{
		winmdroot.Foundation.HANDLE hObjectToSignalLocal;
		if (hObjectToSignal is object)
		{
			hObjectToSignal.DangerousAddRef(ref hObjectToSignalAddRef);
			hObjectToSignalLocal = (winmdroot.Foundation.HANDLE)hObjectToSignal.DangerousGetHandle();
		}
		else
			throw new ArgumentNullException(nameof(hObjectToSignal));
		winmdroot.Foundation.HANDLE hObjectToWaitOnLocal;
		if (hObjectToWaitOn is object)
		{
			hObjectToWaitOn.DangerousAddRef(ref hObjectToWaitOnAddRef);
			hObjectToWaitOnLocal = (winmdroot.Foundation.HANDLE)hObjectToWaitOn.DangerousGetHandle();
		}
		else
			throw new ArgumentNullException(nameof(hObjectToWaitOn));
		winmdroot.Foundation.WAIT_EVENT __result = PInvoke.SignalObjectAndWait(hObjectToSignalLocal, hObjectToWaitOnLocal, dwMilliseconds, bAlertable);
		return __result;
	}
	finally
	{
		if (hObjectToSignalAddRef)
			hObjectToSignal.DangerousRelease();
		if (hObjectToWaitOnAddRef)
			hObjectToWaitOn.DangerousRelease();
	}
}

This meant that some logic could be executed before validating an incoming argument. Moved validation to the top of the method to avoid that. This also gives us an ability to use more idiomatic ArgumentNullException.ThrowIfNull() API on modern .NET versions

Copy link
Member

@jevansaks jevansaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! I'm a little unclear on the benefit of this change. It seems like this will only be observable if a caller passes invalid arguments and catches ArgumentNullException? Argument exception should be fatal so if there's state that hasn't been fully unwound that seems mostly academic. Am I missing something?

public void NullCheckOfSafeHandles_ModernOverload()
{
var expectedCodeGen = """
/// <inheritdoc cref="SignalObjectAndWait(winmdroot.Foundation.HANDLE, winmdroot.Foundation.HANDLE, uint, winmdroot.Foundation.BOOL)"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests shouldn't care what the codegen looks like exactly, we should evaluate observable effects. In other words these might be more suited for the "GenerationSandbox" tests where you exercise some runtime behavior of the change.

@jevansaks
Copy link
Member

Also, in a different scenario where I'm trying to use IWbemLocator.GetObject, it seems that there are many cases where "handles" can be NULL and the failure on null here seems aggressive (see microsoft/win32metadata#2156 also).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants