Add System.Threading.Lock

[Tyrrrz]

No description provided.

[MarkCiliaVincenti]

This lock class is not hardened against thread aborts coming from framework versions preceding .NET 5.0.

Consider using Backport.System.Threading.Lock instead of this method for locking, because it allows you to use the methods that the System.Threading.Lock class provides.

[Tyrrrz]

@MarkCiliaVincenti can you please elaborate on what you mean? I don't see substantial difference between this implementation and https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock/blob/c28041f1e22e561d5cde040704abeeb8d9a18649/Backport.System.Threading.Lock/Lock.cs#L15-L114

[MarkCiliaVincenti]

You're looking at the wrong file. Look at PreNet5Lock.cs and LockFactory.cs.

In fact I made that file you linked only available to net5 and upwards so if someone is on something lower they would not be able to use it.

[MarkCiliaVincenti]

It may not look like much of a difference either but the modern compilers will compile lock (myObj) on a System.Threading.Lock in a way that's not hardened against thread aborts on frameworks that support thread aborts. That's why the factory exists. A lock on Backport.System.Threading.Lock works differently since it is on a different name space.

[Tyrrrz]

You're looking at the wrong file. Look at PreNet5Lock.cs and LockFactory.cs.

In fact I made that file you linked only available to net5 and upwards so if someone is on something lower they would not be able to use it.

I see now. I assume you're mainly referring to this part, right?

https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock/blob/c28041f1e22e561d5cde040704abeeb8d9a18649/Backport.System.Threading.Lock/PreNet5Lock.cs#L114-L124

For what's it worth, this library (PolyShim) just exists, so I can drop in one NuGet package and write the same code across all target frameworks. I'm okay with the polyfills not being a perfect 1-to-1 replacement in terms of performance or edge cases.

[MarkCiliaVincenti]

No it's mainly the normal use case actually, locking on the object itself that is unsafe against thread aborts. It's why I needed the factory, so on frameworks before net9 they're given a return object that's on a different namespace.

[Tyrrrz]

You could've had them both in the same namespace though, right? Since these two types never appear at the same time.

[MarkCiliaVincenti]

The proper backport/polyfill is the one like yours. It's neat and all. Alas, evil thread aborts exists and these are locks we're talking about. Correctness is key in a library, but especially for locks because there exists a race condition of sorts whereby the lock of an aborted thread remains locked and no other threads can obtain the lock.

[Tyrrrz]

Yeah that's fair. I'll update the code on .NET FX to include the try/catch you have in your implementation and credit you in the comments. Thanks for the feedback!

[MarkCiliaVincenti]

Cool, but are you going to change the namespace? If you call it System.Threading.Lock, then you will still have problems.

[Tyrrrz]

Which problems are you referring to?

[MarkCiliaVincenti]

The main problem you have @Tyrrrz is that with your current code:

private static readonly object _syncLock = new();

...

lock (_syncLock)
{
   doThis();
}

does not have the same behaviour as this on versions of .NET before .NET 5.0:

private static readonly System.Threading.Lock _syncLock = new(); // this is from your polyfill

...

lock (_syncLock)
{
   doThis();
}

The object one gets lowered to this:

bool lockTaken = false;
try
{
  Monitor.Enter(syncObject, ref lockTaken);
  doThis();
}
finally
{
  if (lockTaken)
  {
    Monitor.Exit(syncObject);
  }
}

but the polyfilled one because it uses the System.Threading.Lock namespace that's 'reserved' will get lowered to different code. Something like this:

System.Threading.Lock.Scope scope = syncObject.EnterScope();
try
{
  doThis();
}
finally
{
  scope.Dispose();
}

In the above code, if the thread is aborted before it enters the try, i.e. right after obtaining the lock, it will never go into that finally and dispose cleanly, which means that it retains the lock and other threads would be unable to enter the lock as it would be left permanently locked for the lifetime of that lock instance.

[Tyrrrz]

But syncObject.EnterScope() already does Monitor.Enter(syncObject, ref lockTaken) as that's what it's meant to do in the polyfilled implementation.

[MarkCiliaVincenti]

I think the idea is to make the rudimentary lock work exactly like locking on an object would work pre net9. So yes it might work (though do check), but it still feels wrong. What if the lowering changes?

[MarkCiliaVincenti]

@Tyrrrz I confirmed it does call the EnterScope. Unfortunately when code gets lowered this way on < .NET 5.0, if the thread is aborted after the EnterScope and before entering the try, it will fail to dispose. This is not the case if you use a different namespace as I did in PreNet5Lock.cs. In a nutshell, you are still not hardened against thread aborts. Nothing using EnterScope is. This is why I had added this warning on the EnterScope method:

[Obsolete("This method is a best-effort at hardening against thread aborts, but can theoretically retain lock on pre-.NET 5.0. Use with caution.")]

[MarkCiliaVincenti]

There's also another issue. If you create a .NET 8.0 library (let's call it 'ABC') that depends on this project, and then create a new .NET 9.0 project (let's call it 'DEF') that depends on 'ABC', you would get an error because DEF will get the .NET 9.0 version of your library (that would lack the System.Threading.Lock) and the library 'ABC' can't find the class it's meant to use.

My library solves this problem, because it is standalone targeting just System.Threading.Lock. It also solves the problem of thread aborts using a factory method.

Would you be willing to add a dependency to this micro-library?

[Tyrrrz]

I think the idea is to make the rudimentary lock work exactly like locking on an object would work pre net9. So yes it might work (though do check), but it still feels wrong. What if the lowering changes?

Whichever way the lowering changes, it would still have to call EnterScope(...), would it not?

@Tyrrrz I confirmed it does call the EnterScope. Unfortunately when code gets lowered this way on < .NET 5.0, if the thread is aborted after the EnterScope and before entering the try, it will fail to dispose. This is not the case if you use a different namespace as I did in PreNet5Lock.cs. In a nutshell, you are still not hardened against thread aborts. Nothing using EnterScope is. This is why I had added this warning on the EnterScope method:

[Obsolete("This method is a best-effort at hardening against thread aborts, but can theoretically retain lock on pre-.NET 5.0. Use with caution.")]

Well, if the execution gets inside EnterScope() then this part that you wrote will take care of it:

PolyShim/PolyShim/Net90/Lock.cs

Lines 50 to 56 in e56ec5f

	catch (ThreadAbortException)
	{
	if (acquiredLock)
	Monitor.Exit(this);
	throw;
	}

It doesn't need Dispose() to be called.

Also, I cannot provide this type under a different namespace due to the nature of this project -- it contains polyfills which replace native types on frameworks that don't have them. Using the same namespace is imperative.

There's also another issue. If you create a .NET 8.0 library (let's call it 'ABC') that depends on this project, and then create a new .NET 9.0 project (let's call it 'DEF') that depends on 'ABC', you would get an error because DEF will get the .NET 9.0 version of your library (that would lack the System.Threading.Lock) and the library 'ABC' can't find the class it's meant to use.

My library solves this problem, because it is standalone targeting just System.Threading.Lock. It also solves the problem of thread aborts using a factory method.

Would you be willing to add a dependency to this micro-library?

That would not happen because this library is not compiled. The polyfills are included as source code files and every single type provided as internal, so there aren't going to be any conflicts.

First of all, the DEF project won't get this library by transitive means. But even if it installs it anyway, then the Lock polyfill will be turned off. ABC will then have the polyfilled Lock type which is not exposed beyond ABC. The #ifs are evaluated separately for each assembly.

[MarkCiliaVincenti]

If the thread is aborted after the EnterScope and before the try, which is absolutely possible, then the code as is will still retain the lock. The two processes are not atomic.

[Tyrrrz]

If the thread is aborted after the EnterScope and before the try, which is absolutely possible, then the code as is will still retain the lock. The two processes are not atomic.

If the thread is aborted after the EnterScope() but before the try then the lock will not be acquired in the first place since it's acquired within the try.

[MarkCiliaVincenti]

No it isn't. Look again. Not at my code, but at the code I said it would be inlined into. It first calls EnterScope outside of a try-finally, then goes into a try. If the thread is aborted inside the EnterScope or inside the try that happens after the EnterScope, you're safe. But there is still the theoretical race condition that it is aborted exactly after the EnterScope method returns.

[Tyrrrz]

Call trace:

> System.Threading.Lock.Scope scope = syncObject.EnterScope();
try
{
  doThis();
}
finally
{
  scope.Dispose();
}

On .NET 9+ this resolves to some CLR calls.

On .NET 8 or lower this resolves to this:

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

        throw;
    }
}

The lock is ever obtained when Monitor.Enter(...) is called, which is covered by a try/catch. The callsite will lead to this implementation due to the polyfill.

[MarkCiliaVincenti]

You keep focusing on the code inside EnterScope. I agree that the code inside EnterScope is hardened against thread aborts now (it wasn't before), but there is still another issue.

Write this code. You can, after all that is what lock (syncObject) will get lowered into.

System.Threading.Lock.Scope scope = syncObject.EnterScope();
try
{
  doThis();
}
finally
{
  scope.Dispose();
}

Now put a breakpoint on the try (line 2 above). You will see that the code can arrive there.

Now let's change the code a bit, let's do this:

System.Threading.Lock.Scope scope = syncObject.EnterScope();
throw new Exception("this is an experiment to simulate an ill-timed thread abort");
try
{
  doThis();
}
finally
{
  scope.Dispose();
}

If you put a breakpoint in that scope.Dispose();, it will never hit that breakpoint, because it never entered the try (which is now on line 3).

[Tyrrrz]

syncObject.EnterScope() <- how is this implemented?

[MarkCiliaVincenti]

syncObject.EnterScope() <- how is this implemented?

It absolutely does NOT matter except that it returns a disposable object. A disposable object that is not disposed if an exception happens right after such object is returned.

My apologies Oleksii, a combination of me running high fever and talking about a subject I only recently found out about is resulting in me being unable to convey my point.

Let me instead link you to where I learned about this, right on the github issue to introduce the Lock class.
dotnet/runtime#34812 (comment)

Look in particular at KalleOlaviNiemitalo's replies to me.

[Tyrrrz]

It absolutely does NOT matter except that it returns a disposable object. A disposable object that is not disposed if an exception happens right after such object is returned.

But we know how it's implemented, because the implementation is provided by the polyfill on the versions of the framework where thread abortion matters.

Ultimately, like I said already, I can't claim a different namespace, it would defeat the purpose of the library. So if thread abortion is an issue, it'll have to be something I'll live with.

My apologies Oleksii, a combination of me running high fever and talking about a subject I only recently found out about is resulting in me being unable to convey my point.

No problem, hope you feel better.