Add Lock

[SimonCropp]

fixes #212

[MarkCiliaVincenti]

This is not hardened against thread aborts on < .NET 5.0.

Basically someone on .NET Core 3.1 (or earlier) could lock on this class instead of lock on an object and due to a thread abort the lock remains taken and never released, and other threads would not be able to enter.

[SimonCropp]

@MarkCiliaVincenti happy to accept a PR that improves it

[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?

[SimonCropp]

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.

polyfills are internal. so i dont think this is a problem

[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.

polyfills are internal. so i dont think this is a problem

Refer to https://github.com/SimonCropp/Polyfill?tab=readme-ov-file#consuming-in-an-app

Would such a process not make the System.Threading.Lock public without type forwarding?

[SimonCropp]

Would such a process not make the System.Threading.Lock public without type forwarding?

yes. but as it states, it is about "consuming in an app" so in that case type forwarding wont apply.

eg if u target net 8 u get the lock polyfill, if u target 9 u get the proper implementation.

the same applies when using Backport.System.Threading.Lock from within an app

[MarkCiliaVincenti]

Would such a process not make the System.Threading.Lock public without type forwarding?

yes. but as it states, it is about "consuming in an app" so in that case type forwarding wont apply.

It's not very clear. There should be warnings against using it in a library. https://github.com/SimonCropp/Polyfill?tab=readme-ov-file#consuming-in-a-library implies you can use it in a library, at least the way I read it.
Plus it's still risky, imagine someone using Polyfill in an app and then a year later moves the code to a class library and doesn't test it on older frameworks.

eg if u target net 8 u get the lock polyfill, if u target 9 u get the proper implementation.

the same applies when using Backport.System.Threading.Lock from within an app

Not really, this is fixed by type forwarding. I managed to replicate the issue, then created this and confirmed it fixed it:
https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock/blob/master/Backport.System.Threading.Lock/Properties/AssemblyInfo.cs

The problem is that in your case you don't want to apply assembly-level instructions since Polyfill doesn't just backport System.Threading.Lock.

[SimonCropp]

yeah thats a valid scenario where a lib works better than a source only package.

unfortunately most people building libs would prefer to avoid the hassle of transitive dependencies

so again. i am happy consider any PRs that improve the lock implementation

[MarkCiliaVincenti]

The only ways I can think of that fix both problems are:

1. Copy the code from Backport.System.Threading.Lock into its own project and make a reference to it from Polyfill (perhaps using internalsvisibleto)
2. Add a dependency to Backport.System.Threading.Lock and remove Lock.cs

[dahlbyk]

Out of curiosity, what isn't covered by this #if?

[MarkCiliaVincenti]

Probably .NET Framework 4.0 and earlier. There was no method IsEntered in the Monitor class, hence there's no means to use the Monitor class and get the equivalent of IsHeldByCurrentThread.

[dahlbyk]

Earliest supported is net461

Polyfill/readme.md

Lines 8 to 12 in 34a475e

	The package targets `netstandard2.0` and is designed to support the following runtimes.
	* `net461`, `net462`, `net47`, `net471`, `net472`, `net48`, `net481`
	* `netcoreapp2.0`, `netcoreapp2.1`, `netcoreapp3.0`, `netcoreapp3.1`
	* `net5.0`, `net6.0`, `net7.0`, `net8.0`, `net9.0`

[MarkCiliaVincenti]

Sorry, I misread.

Yes, in that case the use of IsHeldByCurrentThread is available. It is available since .NET Standard 1.0 (.NET Framework 4.5)

[dahlbyk]

Am I missing something or is the "hardening" just pulling over the conditional implementation of EnterScope() (which could certainly acknowledge @MarkCiliaVincenti and https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock)?

Otherwise I tend to agree with @SimonCropp that source-only is an appeal of this particular project. Type-forwarding issues aren't unique to Lock here, are there? If anything, I wonder if this project might ship with a conditional AssemblyInfo.cs to forward types only if PolyPublic?

[MarkCiliaVincenti]

Two things:

1. you don't forward the class. You forward the assembly. So unless a polyfill library is only polyfilling classes on a single namespace, it starts getting messy.

2. the code is lowered, on .NET Core 3.1 for example, into a call to EnterScope and setting the return value (a disposable) to a variable immediately followed by a try-finally where in the finally the object is disposed.

However if the thread is aborted right after EnterScope returns and before it goes inside the try (small time window, but it exists), then the object (and hence the lock) is not disposed.

So even if the code inside EnterScope is hardened against thread aborts (that is indeed possible), you have a time window between the time it returns and the time it goes inside the try-finally where it's out of your hands.

[SimonCropp]

note that the type forwarding is only helpful until .net9 is released. at that point libraries can multi-target net9, and then the polyfill will not be used. if the consuming app is using net 9, then the proper implementation will be used. this is the same as with type forwarding

[SimonCropp]

FWIW, the majority of my nugets already target net9, to minimize polyfill usage for net9 targeting apps