If an attempt to acquire a System.Threading.Lock ends up throwing, we should leave the lock unlocked.

[VSadov]

Noticed while reviewing #97227

The acquisition path tries to acquire the lock under try

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs

Line 459 in ca1b161

try

I assume the purpose of that is to catch exceptions that may come from the event source, which is external code.
Since we may see exception after the lock is already in acquired state, the catch should release the lock, but instead it unregisters a waiter.

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs

Lines 544 to 548 in ca1b161

	catch // run this code before exception filters in callers
	{
	State.UnregisterWaiter(this);
	throw;
	}

I think it would be unexpected for the caller that a failed attempt to acquire could leave the lock in a locked state. Worst case the calling thread will terminate while leaving behind an acquired lock, which is UB.

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

Issue Details

---

Noticed while reviewing #97227

The acquisition path tries to acquire the lock under try

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs

Line 459 in ca1b161

try

I assume the purpose of that is to catch exceptions that may come from the event source, which is external code.
Since we may see exception after the lock is already in acquired state, the catch should release the lock, but instead it unregisters a waiter.

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Lock.cs

Lines 546 to 547 in ca1b161

	State.UnregisterWaiter(this);
	throw;

I think it would be unexpected for the caller that a failed attempt to acquire could leave the lock in a locked state. Worst case the calling thread will terminate while leaving behind an acquired lock, which is UB.

Author:	VSadov
Assignees:	-
Labels:	area-System.Threading, untriaged
Milestone:	-

[VSadov]

CC: @kouvel

[kouvel]

I'm thinking once the lock is acquired any exceptions (there shouldn't be any important ones) can be ignored. Could consider undoing the state changes and propagating the exception but it seems a rare need and unnecessarily more complicated.

[VSadov]

The bug is indirectly a result of the complexity in the lock (various nested loops, etc..).
Fixing this issue would require refactoring and would be safer to do in 11.

Considering that this issue has been around for two releases and has not been observed to cause actual failures, I think at this point in release cycle pushing this to 11 is a better option.