Fix SemaphoreSlim validation in Wait/WaitAsync methods

[eduardo-vp]

Fix range validation for some Wait/WaitAsync methods that take the timeout as an integer.

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull Request Overview

This PR adds input validation for the millisecondsTimeout parameter to two overloads of Wait and WaitAsync methods in SemaphoreSlim, bringing them into alignment with other overloads that already perform this validation.

• Added ArgumentOutOfRangeException validation for negative timeout values (other than -1) in Wait(int millisecondsTimeout) and WaitAsync(int millisecondsTimeout)
• Added inline comments explaining the validation and subsequent method calls

[jkotas]

What is the bug that this is fixing? Do we need to add tests for it?

[eduardo-vp]

Those methods don't have validation now and are allowed to actually wait even if the millisecondsTimeout argument is less than -1.

The bug should have been caught in:

runtime/src/libraries/System.Threading/tests/SemaphoreSlimTests.cs

Lines 66 to 71 in 3083294

	[Fact]
	public static void RunSemaphoreSlimTest1_Wait_NegativeCases()
	{
	// Invalid timeout
	RunSemaphoreSlimTest1_Wait_Helper(10, 10, -10, true, typeof(ArgumentOutOfRangeException));
	}

runtime/src/libraries/System.Threading/tests/SemaphoreSlimTests.cs

Lines 199 to 225 in 3083294

	private static void RunSemaphoreSlimTest1_Wait_Helper
	(int initial, int maximum, object timeout, bool returnValue, Type exceptionType)
	{
	SemaphoreSlim semaphore = new SemaphoreSlim(initial, maximum);
	try
	{
	bool result = false;
	if (timeout is TimeSpan)
	{
	result = semaphore.Wait((TimeSpan)timeout);
	}
	else
	{
	result = semaphore.Wait((int)timeout);
	}
	Assert.Equal(returnValue, result);
	if (result)
	{
	Assert.Equal(initial - 1, semaphore.CurrentCount);
	}
	}
	catch (Exception ex)
	{
	Assert.NotNull(exceptionType);
	Assert.IsType(exceptionType, ex);
	}
	}

Wait didn't throw an exception, it continued the test execution and asserted that the wait was successful and the count decremented.

In this case the test can be fixed by updating the expected result of Wait to false such that if the semaphore successfully waits, an exception will be thrown.

However the problem is that the code was expected to throw but it didn't and it never checked that the exception was not thrown. I believe this could potentially happen for other tests too. Shall we update the tests pattern to verify this?

[eduardo-vp]

We do validate millisecondsTimeout in all the Wait/WaitAsync methods, only these two were missing the validation.

[eduardo-vp]

/ba-g unrelated errors, just adding some validation to some methods