Skip to content

Fix SemaphoreSlim validation in Wait/WaitAsync methods#121289

Merged
eduardo-vp merged 4 commits intodotnet:mainfrom
eduardo-vp:fix-semaphore-slim-validation
Nov 3, 2025
Merged

Fix SemaphoreSlim validation in Wait/WaitAsync methods#121289
eduardo-vp merged 4 commits intodotnet:mainfrom
eduardo-vp:fix-semaphore-slim-validation

Conversation

@eduardo-vp
Copy link
Member

@eduardo-vp eduardo-vp commented Nov 3, 2025 •
edited
Loading

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

Copilot AI review requested due to automatic review settings November 3, 2025 08:03
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 3, 2025
@eduardo-vp eduardo-vp added area-System.Threading and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 3, 2025
@dotnet-policy-service
Copy link
Contributor

dotnet-policy-service bot commented Nov 3, 2025

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

@eduardo-vp eduardo-vp changed the title Fix SemaphoreSlim validation in constructors Fix SemaphoreSlim validation in Wait methods Nov 3, 2025
Copilot AI reviewed Nov 3, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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

eduardo-vp and others added 2 commits November 3, 2025 00:16
@eduardo-vp
Update src/libraries/System.Private.CoreLib/src/System/Threading/Sema…
2795e80 
…phoreSlim.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@eduardo-vp
Update src/libraries/System.Private.CoreLib/src/System/Threading/Sema…
fea7246 
…phoreSlim.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@eduardo-vp eduardo-vp changed the title Fix SemaphoreSlim validation in Wait methods Fix SemaphoreSlim validation in Wait/WaitAsync methods Nov 3, 2025
This was referenced Nov 3, 2025
Open
Open
@jkotas
Copy link
Member

jkotas commented Nov 3, 2025

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

@eduardo-vp
Copy link
Member Author

eduardo-vp commented Nov 3, 2025 •
edited
Loading

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
Copy link
Member Author

eduardo-vp commented Nov 3, 2025 •
edited
Loading

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

@eduardo-vp eduardo-vp requested a review from jkotas November 3, 2025 18:10
@eduardo-vp
Copy link
Member Author

eduardo-vp commented Nov 3, 2025

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

@eduardo-vp eduardo-vp merged commit 3023763 into dotnet:main Nov 3, 2025
133 of 142 checks passed
@dotnet-maestro dotnet-maestro bot mentioned this pull request Nov 6, 2025
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot Copilot left review comments

@jkotas jkotas jkotas approved these changes

Assignees

@eduardo-vp eduardo-vp

Labels

area-System.Threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@eduardo-vp @jkotas