Add first class System.Threading.Lock type

[benaadams]

Background and Motivation

Locking on any class has overhead from the dual role of the syncblock as both lock field and hashcode et al. (e.g. #34800)

Adding a first class lock type that didn't allow alternative uses and only acted as a lock would allow for a simpler and faster lock as well as be less ambiguous on type and purpose in source code.

API proposal

[Edit] by @kouvel based on #34812 (comment) and dotnet/csharplang#7104

namespace System.Threading
{
    // This is a lock that can be entered by one thread at a time. A thread may enter the lock recursively multiple
    // times, in which case the thread should also exit the lock the same number of times to fully exit the lock.
    public sealed class Lock
    {
        // Exceptions:
        //   - LockRecursionException if the lock was entered recursively by the current thread the maximum number of times
        public void Enter();

        // Returns true if the lock was acquired by the current thread, false otherwise.
        // Exceptions:
        //   - LockRecursionException if the lock was entered recursively by the current thread the maximum number of times.
        //     In this corner case an exception would be better than returning false, as the calling thread cannot make
        //     further progress with entering the lock before exiting it.
        public bool TryEnter();

        // Returns true if the lock was acquired by the current thread, false otherwise.
        // Exceptions:
        //   - ArgumentOutOfRangeException if the timeout value converted to an integer milliseconds value is negative
        //     and not equal to -1, or greater than Int32.MaxValue
        //   - LockRecursionException if the lock was entered recursively by the current thread the maximum number of times
        //     In this corner case an exception would be better than returning false, as the calling thread cannot make
        //     further progress with entering the lock before exiting it.
        public bool TryEnter(TimeSpan timeout);

        // Returns true if the lock was acquired, false otherwise.
        // Exceptions:
        //   - ArgumentOutOfRangeException if the timeout value is negative and not equal to -1
        //   - LockRecursionException if the lock was entered recursively by the current thread the maximum number of times
        //     In this corner case an exception would be better than returning false, as the calling thread cannot make
        //     further progress with entering the lock before exiting it.
        public bool TryEnter(int millisecondsTimeout);

        // Exceptions: SynchronizationLockException if the current thread does not own the lock
        public void Exit();

        // Returns true if the current thread holds the lock, false otherwise.
        //
        // This is analogous to Monitor.IsEntered(), but more similar to SpinLock.IsHeldByCurrentThread. There could
        // conceivably be a Lock.IsHeld as well similarly to SpinLock.IsHeld, or Lock.IsHeldByAnyThread to be more
        // precise, which would return true if the lock is held by any thread.
        public bool IsHeldByCurrentThread { get; }

        // Enters the lock and returns a holder. Enables integration with the "lock" keyword.
        // Exceptions:
        //   - LockRecursionException if the lock was entered recursively by the current thread the maximum number of times
        public Scope EnterScope();

        public ref struct Scope
        {
            // Exits the lock. No-op if Dispose was already called.
            // Exceptions: SynchronizationLockException if the current thread does not own the lock
            public void Dispose();
        }
    }
}

API Usage

Change in usage

private object _lock = new object();

Becomes the clearer

private Lock _lock = new Lock();

[Edit] by @kouvel

Usage example 1

_lock.Enter();
try
{
    // do something
}
finally
{
    _lock.Exit();
}

Usage example 2

using (_lock.EnterScope())
{
    // do something
}

Usage example 3

After the lock statement integration described in #34812 (comment), the following would use Lock.EnterScope and Scope.Dispose instead of using Monitor.

lock (_lock)
{
    // do something
}

TryEnterScope usage example

The following:

if (_lock.TryEnter())
{
    try
    {
        // do something
    }
    finally { _lock.Exit(); }
}

Could be slightly simplified to:

using (var tryScope = _lock.TryEnterScope())
{
    if (tryScope.WasEntered)
    {
        // do something
    }
}

The latter could also help to make the exit code path less expensive by avoiding a second thread-static lookup for the current thread ID.

Alternative Designs

• Scope could be a regular struct instead of a ref struct. A ref struct helps to make the exit code path less expensive and can improve performance in some cases, as it guarantees that Scope.Dispose is called on the same thread as EnterScope, and the path that exits the lock would be able to avoid an extra thread-static lookup for the current thread ID. Similarly for TryScope.
• For the TryEnterScope methods, an alternative is to return a nullable struct. It seems more readable to check for WasEntered than to check for null or the HasValue property to see if the lock was entered. Also a ref struct can't be nullable currently, and a regular struct would not benefit from a potentially less expensive exit path.
• IsHeld instead of IsHeldByCurrentThread. The former is simpler, though may be a bit ambiguous, and may be confusing against SpinLock.IsHeld, where it means IsHeldByAnyThread.
• Making the Lock type a struct instead. This may save some allocations in favor of increasing the size of the containing type. Structs can be a bit more cumbersome to work with, and the benefits may not be large enough to be worthwhile.

Risks

• There may be some changes based on the language proposal in [Proposal]: Lock statement pattern (VS 17.10, .NET 9) csharplang#7104

[Joe4evr]

Should this type not be sealed?
Also, should there be some in-box analyzers shipped alongside? I can think of:

• Indicate this type's only purpose is to be used for locks
• Suggestion that places where a field is only used to be locked on, that field's type can be changed to this

[benaadams]

Should this type not be sealed?

Yep added, thanks

[stephentoub]

I'm not clear on the extent of this proposal.

You're proposing that Monitor would special-case this with its existing object-based methods? But only some of Monitor's methods?

[benaadams]

I'm not clear on the extent of this proposal.

I'm proposing a type that doesn't need to concern itself with the state of the syncblock so it will only be a thinlock and not worry about handling the flags BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX, BIT_SBLK_IS_HASHCODE, BIT_SBLK_SPIN_LOCK etc

[Wraith2]

So no special treatment anywhere in the runtime it's just setup so that it'll never have a stored hashcode and the syncblock will only ever be used for locking? Though it'd be a pretty niche case where you have a lock on something that has been hashed i'd have thought.

[stephentoub]

I'm proposing a type that doesn't need to concern itself with the state of the syncblock so it will only be a thinlock and not worry about handling the flags

I understand. But how are you proposing interacting with it? Still via Monitor?

[benaadams]

Ah, get you point. Have added other methods.

Would still need to go via monitor currently to work with the lock statement?

[benaadams]

Though it'd be a pretty niche case where you have a lock on something that has been hashed i'd have thought.

Yes; but the handling for the niche case isn't without overhead. So the idea is to introduce a type that drops this overhead by being explicit in only being a lock.

[stephentoub]

Would still need to go via monitor currently to work with the lock statement?

Today, yes, though likely C# could be augmented to recognize the type in the future.

So the idea is to introduce a type that drops this overhead by being explicit in only being a lock.

A potentially much more impactful (and experimental) variation of this would be to actually only support locking on such a type and eliminate the memory overhead incurred for every other object in the system. Until code fully transitioned, locking on other objects would obviously be more expensive as the runtime would need to employ some kind of mapping to a Lock, e.g. https://github.com/dotnet/corert/blob/d0683071605aed956497506ed1aae4d366be0190/src/System.Private.CoreLib/src/System/Threading/Monitor.cs#L28

[GrabYourPitchforks]

/cc @davidwrighton as FYI since we were coincidentally talking about this just the other day.

[davidwrighton]

:) This brings back memories of the early days on the .NET Native project, when we actually implemented this type, and tied it into Monitor (so uses of this Lock type were really great, and other objects had a perf penalty due to the cost of checking for the Lock type. We eventually concluded penalizing all existing Monitor.Lock usage (even slightly) was a poor choice, and reverted back to supporting arbitrary object locking as the first class primitive. One idea I had at the time, which I never explored in detail was the interesting fact that in CoreCLR, there is an empty and unused pointer sized field in every System.Object instance. In theory, if optimizing Monitor performance really only needs a single non-gc tracked pointer field, and most locking occurs on instances of the exact System.Object type, we could use that instead. This would have the same drawbacks a an integrated Lock object for reducing the performance of locking on other object types, but we've been telling customers for years to lock on System.Object instances. Perhaps we could find out if that would be a worthwhile optimization.

If we want to have a lock object type not integrated into Monitor, then there is quite a bit of work required to make sure it integrates with the diagnostics infrastructure and the threading model correctly. (Monitor is an interruptible lock as it uses a COM interruptible wait so that STA frameworks such as WinForms and WPF work correctly. Any other lock that is intended for general use will also need to follow those strictures.)

[stephentoub]

in CoreCLR, there is an empty and unused pointer sized field in every System.Object instance

Why? What prevents us from just removing that?

[benaadams]

interesting fact that in CoreCLR, there is an empty and unused pointer sized field in every System.Object instance.

Is this due to the min size of 12 bytes?

runtime/src/coreclr/src/vm/object.h

Lines 108 to 111 in c06810b

	// The generational GC requires that every object be at least 12 bytes
	// in size.
	#define MIN_OBJECT_SIZE (2*TARGET_POINTER_SIZE + OBJHEADER_SIZE)

This would have the same drawbacks a an integrated Lock object for reducing the performance of locking on other object types, but we've been telling customers for years to lock on System.Object instances.

What would be performance the drawback? Due to an extra _lock.GetType() == typeof(object) check to determine the code path?

[jkotas]

.NET Native project, when we actually implemented this type, and tied it into Monitor

This implementation is still there .

In CoreCLR, the lock can take two paths: The thinlock path that just flips the bit in the object header is used as long as there is no contention that requires waiting or other special situations. Once the thin lock hits the contention, the syncblock (a piece of unmanaged memory) is allocated for the object by the VM, and all lock operations go through the syncblock. Going through syncblock has extra overhead - there are several indirections required to get to syncblock.

In CoreRT, the thinlock path is same as in CoreCLR (the thin lock path was not there in the early .NET Native days). Once the lock hits contention, one of these Lock objects gets attached to the object via ConditionalWeakTable. The Lock object + ConditionalWeakTable is basically a replacement for syncblock as far as Monitor is concerned. I believe that the same strategy would work for CoreCLR too.

min size of 12/24 bytes

Here is where the min size comes into play: The current GC implementation needs to be able replace any chunk of free space with a special fake array to keep the heap walk-able. The object minimum size is a minimum size of this fake array. If we were to get rid of the free space on System.Object, we would need to introduce a special fake object that would be used for free space where the fake array does not fit, and all places in the GC that check for the free space would need to check for both these. Some of these checks are in hot loops that would take a perf hit.

The technically most challenging part for getting rid of object header is dealing with object hashcodes (ie RuntimeHelpers.GetHashCode) during GC object relocation. Many other components depend on these hashcodes, e.g. ConditionalWeakTable. Until we have satisfactory solution for that, the rest does not matter.

[kouvel]

What about making the Lock type a struct, analogous to SpinLock? Perhaps it could save some allocations, and it could potentially be integrated into the lock statement while keeping it separate from locking on arbitrary objects with lock (ref _lock) or even just lock (_lock) with implied passing by ref particularly for the Lock type.

[VSadov]

The current implementation of NativeAOT locking is here:

• Thin lock that uses the sync bits in the object header.
  This lock does not allocate any additional memory. It is also very fast. In common scenarios like singlethreaded use or low-contention it can do 1 billion lock/unlock cycles in 5 seconds.
  However it cannot handle certain scenarios, like sleeping waits due to contention or if the locking object needs to also store a hashcode. In such scenarios the thin lock is automatically "inflated" into:
• Actual System.Threading.Lock that supports all scenarios expected from a recursive mutex.
  This is a hybrid lock that will do a blocking wait after a bit of spinning.
  While short-held locks benefit from spinning, for longer-held locks spinning is wasteful and more eager sleeping saves resources. The Lock will detect these cases and self-adjust based on the past history of the lock.

NativeAOT managed runtime often bypasses the thin lock stage and just uses Lock object directly. That makes sense for long-lived locks or locks that will inevitably inflate.

I think the NativeAOT Lock could work nicely as a first class Lock type.

[benaadams]

I have low expectations and am happy with a Lock type that inherits from Object and throws on GetHashCode

Currently every class is sorta a lock, but there is no type that expresses the intention, so when you want a vanilla lock you do

object _lock = new object();

Which seems weird; and incongruous with .NET where every api is spelled out very clearly and expresses its intent in naming. That clarity in apis is one of the things .NET is very good at, however its missing for the fundamental lock type, perhaps because everything is a lock?

i.e. to a new .NET developer wanting a lock; the answer isn't "use a lock" but "use any object" which isn't an obvious answer

[Joe4evr]

Currently every class is sorta a lock, but there is no type that expresses the intention, so when you want a vanilla lock you do

object _lock = new object();

Tbf, I think as part of the early early design of the CLR, it was thought that locking on this would be the standard thing to do, which necessitated the ability to make every class instance a lock. However, I guess since the languages didn't restrict the argument to be only this, they ended up with the ability to use other object instances as locks, including objects that already used lock (this) making it easy to deadlock. After that, it became the wisdom that locks should only be taken on owned objects completely invisible to outside consumers, which is where we are today.

[benaadams]

Tbf, I think as part of the early early design of the CLR, it was thought that locking on this would be the standard thing to do, which necessitated the ability to make every class instance a lock. However, I guess since the languages didn't restrict the argument to be only this, they ended up with the ability to use other object instances as locks, including objects that already used lock (this) making it easy to deadlock. After that, it became the wisdom that locks should only be taken on owned objects completely invisible to outside consumers, which is where we are today.

Sure and I'm not saying remove the standard locking mechanism; it is handy when you don't want an additional allocation, just introduce a type that conveys intention rather than using object for when you want an explicit lock object

[theodorzoulias]

I think that the name Lock is problematic for this class. Declaring a Lock field is OK:

private readonly Lock _lock = new();

But declaring a local variable is a compile-time error:

Lock lock = new(); // Syntax error

The lock is a C# keyword. And having to bypass this restriction by naming the variable @lock is a bummer.

[benaadams]

The lock is a C# keyword. And having to bypass this restriction by naming the variable @lock is a bummer.

Would be uncommon? When would you define a local lock? Would only serve a purpose if you where also passing it as a parameter to methods that would execute in parallel otherwise it would have no function, as what is there to lock?

[KalleOlaviNiemitalo]

@MarkCiliaVincenti, the problem could occur on .NET Framework if your code uses an API-compatible reimplementation of System.Threading.Lock but someone else's code (e.g. part of ASP.NET infrastructure) aborts the thread.

[MarkCiliaVincenti]

@weltkante then I'm even understanding less

@KalleOlaviNiemitalo then it would work just the same as lock (myObj) where myObj is of type system.object on .NET Framework. Imagine I made a class called HelloWorld, created an instance of it and locked on that instance, the behaviour on all framework versions is expected to be just the same as an instance of system.object, no?

[KalleOlaviNiemitalo]

@MarkCiliaVincenti, yes, just the System.Threading.Lock name is special-cased in the compiler and causes lock to use an API that might not recover from thread abort.

[weltkante]

@weltkante then I'm even understanding less

Whats not to understand in the explanation of the example? You'll have to be more specific if you have questions.

[...] which leaves syncObject locked if a ThreadAbortException is thrown after Monitor.Enter(this) called by Lock.Enter() has locked syncObject, but before the try block has started.

If your abort happens between the lock request but before the try block then the finally won't be executed and no unlock happens.

[MarkCiliaVincenti]

I think you mean that .NET Framework which would still actually be locking on an object would leave the object unlocked on an abort

I think you got it wrong here, using code equivalent to the new pattern on Desktop Framework leaves it locked, not unlocked. So when you have unlucky timing for your thread aborts (which are actively used by the framework) you get deadlocks on Desktop Framework. On .NET Core it doesn't "leave" it in any state at all because it can't happen.

I see your edit now. This has nothing to do with backporting of System.Threading.Lock though, if I understand you correctly. That is if you have code that is multi-targeting .NET Framework up to .NET 8.0 and locking on a readonly object, then you ALREADY have this problem, no? Or am I still not understanding you?

[weltkante]

If you are locking using the old API a different pattern is used, with the Monitor.Enter inside the try block (and reporting lock state via boolean variable), so you don't have the problem of not executing the finally. Its all in the example/explanation above.

So yes, this is a problem with backporting the new pattern, since it compiles differently (since it wasn't designed for Desktop Framework and thread aborts)

[MarkCiliaVincenti]

But the backport is still locking on an object. Imagine you implement #34812 (comment)

And then you have code like this on .NET Framework:

private readonly System.Threading.Lock myLock = new();
private readonly object myObj = new();

public void LockLock()
{
   lock (myLock)
   {
      // blabla
   }
}

public void LockObject()
{
   lock (myObj)
   {
      // blabla
   }
}

These methods should work exactly the same with .NET Framework and the class from #34812 (comment) compiled in the same assembly, no?

[KalleOlaviNiemitalo]

No, they won't work exactly the same, if you build with a compiler that recognizes the System.Threading.Lock name. lock(myObj) in LockObject() will use Monitor.Enter(object obj, ref bool lockTaken), but lock (myLock) in LockLock() won't.

[MarkCiliaVincenti]

No, they won't work exactly the same, if you build with a compiler that recognizes the System.Threading.Lock name. lock(myObj) in LockObject() will use Monitor.Enter(object obj, ref bool lockTaken), but lock (myLock) in LockLock() won't.

Which .NET Framework version "recognizes the System.Threading.Lock name"?

For .NET Framework, System.Threading.Lock would simply be a class in the same assembly that inherits from System.Object and has no variables, just methods and properties.

Why would it work differently? For .NET Framework, System.Threading.Lock might well have been MyNamespace.MyClass

[weltkante]

Which .NET Framework version "recognizes the System.Threading.Lock name"?

None, its a compiler thing, and the compiler isn't designed to generate code for Desktop Framework in presence of thread aborts (for the new-style locking API). So if you use the new Compiler Feature together with Desktop Framework and Thread Aborts the code the compiler generates will be buggy and lead to deadlocks.

I think you're overlooking that the IL is generated by the compiler, and depends on whether you are using the old or new feature, it is not determined by the framework which will run it later.

[MarkCiliaVincenti]

So if I'm understanding correctly, my examples of LockLock and LockObject, if the .NET 8.0 compiler is used, it's not a problem, but if the .NET 9.0 compiler is used it would have different behaviour, even though ultimately the product is a .NET Framework 4.8 executable?

[KalleOlaviNiemitalo]

The C# compiler in .NET SDK 8.0.400 already recognizes the System.Threading.Lock name and uses the new API, if you have <LangVersion>preview</LangVersion>. Otherwise, it fails with:

error CS8652: The feature 'Lock object' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version.

But if you instead do global using Lock = System.Object;, so that the System.Threading.Lock name is not involved, then it is safe for .NET Framework.

[MarkCiliaVincenti]

I still don't get it.

Are we saying that if you were to do this:

namespace System.Threading;

public class Lock
{
   public void Hello()
   {
      // do nothing
   }
}

and

private readonly System.Threading.Lock myLock = new();

public void Foo()
{
   lock (myLock)
   {
      myLock.Hello();
   }
}

This wouldn't work on .NET 8 because somehow it refuses to use System.Threading.Lock from your assembly but System.Threading.Lock from somewhere else, where it won't have the Hello method?

[KalleOlaviNiemitalo]

On .NET SDK 8.0.400, if you try to build

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
  </PropertyGroup>

</Project>

namespace System.Threading
{
    public class Lock
    {
        public void Hello()
        {
            // do nothing
        }
    }
}

public class LockDemo
{
    private readonly System.Threading.Lock myLock = new System.Threading.Lock();

    public void Foo()
    {
        lock (myLock)
        {
            myLock.Hello();
        }
    }
}

then it fails outright:

LockDemo.cs(18,15): error CS0656: Missing compiler required member 'System.Threading.Lock.EnterScope'

because the compiler recognizes the System.Threading.Lock name and attempts to generate an EnterScope call for lock (myLock), but the System.Threading.Lock type that is defined in this project doesn't have that method.

The same error occurs also if you change the project to target .NET Framework 4.8:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net48</TargetFramework>
  </PropertyGroup>

</Project>

[MarkCiliaVincenti]

Hmm, I managed to replicate the above @KalleOlaviNiemitalo. So there is no way to 'override'?

[KalleOlaviNiemitalo]

I'd consider the System.Threading.Lock name reserved for this purpose, now.
If you implement a lock class with a different API, then give it a different name or place it in a different namespace.

[MarkCiliaVincenti]

I'd consider the System.Threading.Lock name reserved for this purpose, now. If you implement a lock class with a different API, then give it a different name or place it in a different namespace.

So how do I test this? I tried this on net462 in a project that multi-targets even net9.0, and where Lock is https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock/blob/master/Backport.System.Threading.Lock/Lock.cs but both tests passed. What am I doing wrong pls?

    [Fact]
    public void AbortTest()
    {
        var myLock = new Lock();
        var wasAborted = false;
        var thread = new Thread(() => {
            try
            {
                lock (myLock)
                {
                    Thread.Sleep(1000);
                    Thread.Sleep(1000);
                    Thread.Sleep(1000);
                    Thread.Sleep(1000);
                    Thread.Sleep(1000);
                }
            }
            catch (ThreadAbortException abortException)
            {
                wasAborted = true;
            }
        });
        thread.Start();
        Thread.Sleep(2000);
        thread.Abort();
        myLock.IsHeldByCurrentThread.Should().BeFalse();
        wasAborted.Should().BeTrue();
    }

    [Fact]
    public void AbortTestObject()
    {
        var myLock = new object();
        var wasAborted = false;
        var thread = new Thread(() => {
            try
            {
                lock (myLock)
                {
                    Thread.Sleep(1000);
                    Thread.Sleep(1000);
                    Thread.Sleep(1000);
                    Thread.Sleep(1000);
                    Thread.Sleep(1000);
                }
            }
            catch (ThreadAbortException abortException)
            {
                wasAborted = true;
            }
        });
        thread.Start();
        Thread.Sleep(2000);
        thread.Abort();
        Monitor.IsEntered(myLock).Should().BeFalse();
        wasAborted.Should().BeTrue();
    }

[KalleOlaviNiemitalo]

Those Thread.Sleep calls are within the try block that is generated from the lock statement. If you abort the thread during one of those sleeps, then it will release the lock, no problem.

The problem would occur if you abort the thread just as it is about to enter the try block. The time window is small so this may be difficult to reproduce reliably.

[MarkCiliaVincenti]

Those Thread.Sleep calls are within the try block that is generated from the lock statement. If you abort the thread during one of those sleeps, then it will release the lock, no problem.

The problem would occur if you abort the thread just as it is about to enter the try block. The time window is small so this may be difficult to reproduce reliably.

I see, that helps a lot, thanks. I will try to reproduce it.

But what I still don't quite get is this: if you're multi-targeting net462 and net9.0, you will not be using thread.Abort() since net9.0 doesn't support it, so is there an actual problem? Maybe if you pass on a thread to a net462 library that would do the abort itself?

In such a case, then doing the following instead of the lock would the below solve it?

var backportedLock = new System.Threading.Lock();
bool lockTaken = false;
try
{
   lockTaken = backportedLock.TryEnter();
   ...
}
finally
{
   if (lockTaken) backportedLock.Exit();
}

[KalleOlaviNiemitalo]

That would not solve it, because the thread could be aborted after backportedLock.TryEnter() has entered the lock but before the result is assigned to the lockTaken variable. This is why Monitor.Enter has a ref bool lockTaken parameter rather than a return value.

[MarkCiliaVincenti]

That would not solve it, because the thread could be aborted after backportedLock.TryEnter() has entered the lock but before the result is assigned to the lockTaken variable. This is why Monitor.Enter has a ref bool lockTaken parameter rather than a return value.

This sucks because .Enter() on System.Threading.Lock does not accept the ref bool lockTaken, so it seems like there's no way to mimic it. It's why I tried using TryEnter().

It's a very unlikely scenario but if we're talking about correctness...

I asked you another question as well and I'd like to have some input on that, I'd appreciate it.

"But what I still don't quite get is this: if you're multi-targeting net462 and net9.0, you will not be using thread.Abort() since net9.0 doesn't support it, so is there an actual problem? Maybe if you pass on a thread to a net462 library that would do the abort itself?"

I'm basically trying to understand the exact chain of events that need to happen for the Backport.System.Threading.Lock micro-library to fail, so I can explain it, because I couldn't find another way to fix this, not even using giving the class a different namespace than System.Threading and using TypeForwardedTo to System.Threading with preprocessor directives only for NET9 or greater.

[MarkCiliaVincenti]

How about this?

var backportedLock = new System.Threading.Lock();
try
{
   backportedLock.Enter();
   ... // stuff that's inside the lock
}
catch (ThreadAbortException)
{
   backportedLock.Enter(); // thanks to lock reentrancy will not stay waiting even if lock was obtained
}
finally
{
   backportedLock.Exit();
}

[KalleOlaviNiemitalo]

At https://referencesource.microsoft.com/#q=Thread.Abort, you can search for .NET Framework code that calls Thread.Abort() or Thread.Abort(object stateInfo). Most of those calls look like the thread being aborted is likely to be running only code that is part of .NET Framework. However, the ASP.NET execution timeout will abort a thread on which user code has spent too much time handling an HTTP request. See RequestTimeoutManager.cs and docs for HttpContext.ThreadAbortOnTimeout and HttpRuntimeSection.ExecutionTimeout.

So if you make a library that uses a backported System.Threading.Lock, and someone's ASP.NET application calls your library on request-handler thread without disabling ThreadAbortOnTimeout, then that's not safe.

I think CLR hosting in SQL Server may also abort threads running user code, but I'm not sure about that.

[MarkCiliaVincenti]

At https://referencesource.microsoft.com/#q=Thread.Abort, you can search for .NET Framework code that calls Thread.Abort() or Thread.Abort(object stateInfo). Most of those calls look like the thread being aborted is likely to be running only code that is part of .NET Framework. However, the ASP.NET execution timeout will abort a thread on which user code has spent too much time handling an HTTP request. See RequestTimeoutManager.cs and docs for HttpContext.ThreadAbortOnTimeout and HttpRuntimeSection.ExecutionTimeout.

So if you make a library that uses a backported System.Threading.Lock, and someone's ASP.NET application calls your library on request-handler thread without disabling ThreadAbortOnTimeout, then that's not safe.

I think CLR hosting in SQL Server may also abort threads running user code, but I'm not sure about that.

I see, and if that Lock instance is set to static, then one aborted ASP.NET thread might leave it in a locked state in a very rare but still possible scenario.

It seems like if you want to target both .NET Framework 4.8 and .NET 9.0 and want to use the new System.Threading.Lock and enjoy its speed advantages, you can't get all of these 3:

• ability to use the methods (and performance on .NET 9) provided by System.Threading.Lock
• no performance disadvantage on < .NET 9.0
• no ugly/messy code (maybe with pre-processor directives)

The library does a best effort but it is not perfect.

[MarkCiliaVincenti]

Perhaps the solution might lie in the backport library users avoiding use of lock (myLock) and instead relying on EnterScope(), and EnterScope() changed to something like this (at least on frameworks supporting thread abortion):

    public Scope EnterScope()
    {
        bool lockTaken = false;
        try
        {
           Monitor.Enter(this, ref lockTaken);
           return new Scope(this);
        }
        catch (ThreadAbortException)
        {
           if (lockTaken) Monitor.Exit(this);
        }
    }

and thus instead of doing:

lock (myLock)
{
   ...
}

they would do:

using (myLock.EnterScope())
{
   ...
}

and if I understand correctly then this pattern would be hardened against thread aborts.

[KalleOlaviNiemitalo]

No, that would still not be fully hardened. In using (myLock.EnterScope()), the thread could be aborted after EnterScope() returns but before the result is assigned to the unnamed local variable.

A lock class with a different API could be hardened against thread aborts, but would be too tedious to use without compiler support in lock statements. I don't believe such a language feature would be approved, now that there already are two APIs for lock statements and Thread.Abort is a legacy feature. If you want to use System.Threading.Lock on .NET 9 but support .NET Framework too, I think conditional compilation as in #34812 (comment) is the best solution for now. (Apart from global using requiring C# 10, which is not formally supported on .NET Framework.)

[MarkCiliaVincenti]

No, that would still not be fully hardened. In using (myLock.EnterScope()), the thread could be aborted after EnterScope() returns but before the result is assigned to the unnamed local variable.

Argh!

A lock class with a different API could be hardened against thread aborts, but would be too tedious to use without compiler support in lock statements. I don't believe such a language feature would be approved, now that there already are two APIs for lock statements and Thread.Abort is a legacy feature. If you want to use System.Threading.Lock on .NET 9 but support .NET Framework too, I think conditional compilation as in #34812 (comment) is the best solution for now. (Apart from global using requiring C# 10, which is not formally supported on .NET Framework.)

That solution is not very good. It limits you to using just the lock keyword, nothing else.

Here's how:

#if NET9_0_OR_GREATER
global using Lock = System.Threading.Lock;
#else
global using Lock = System.Object;
#endif

private readonly Lock myObj = new();

void DoThis()
{
   lock (myObj)
   {
      // do something
   }
}

void DoThat()
{
   myObj.Enter(5); // this will not compile on .NET 8.0
   Monitor.Enter(myObj, 5); // this will immediately enter the lock on .NET 9.0 even if another thread is locking on DoThis()
   // do something else
}

I'm thinking that a potential solution is to change https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock/blob/master/Backport.System.Threading.Lock/Lock.cs such that the namespace is BackportSystem.Threading and then make a factory for it.

public static class LockFactory
{
#if NET9_0_OR_GREATER
    public static System.Threading.Lock Create() => new();
#else
    public static BackportSystem.Threading.Lock Create() => new();
#endif
}