System.Threading.Lock

[MarkCiliaVincenti]

Would you be interested if I wrote a PR bringing in a dependency to my library Backport.System.Threading.Lock that will give you better locking performance on .NET 9.0+?

[koszeggy]

Hi Mark,

I'm glad if you find my libraries useful or interesting enough to contribute to it. Basically I'm open to collaborations, though for my CoreLibraries I have a very strict constraint not to have any dependencies to any other packages. I don't reference even System.Memory or System.Buffers that would allow to use spans or ArrayPool<T> in more platform targets to avoid possible version mismatch issues. Of course, this sometimes makes my own life just more complicated when I have to reimplement some functionality for older frameworks that would be available via packages otherwise, but I can live with that.

In my other libraries I don't stick to this rule so much, so for example, my Drawing.Core library references System.Numerics for some targets to be able to use vectors on more platforms. But in some cases it already started to cause issues: my GDI+-specific Drawing package has to reference 3 different versions of the System.Drawing.Common package because Microsoft itself started to drop compatibility with older .NET versions in their newer packages. But I hope this will not lead to Newtonsoft.Json-like unresolvable issues (i.e. when two or more dependent packages reference different versions of the same package with breaking changes between those versions, hence in an non-resolvable way).

If you still would like to improve KGySoft.CoreLibraries, feel free to add the functionality without introducing a new dependency (preferably, by forking the Net9 branch). You can place a comment with your name and/or a link to your original repository if you wish, this is how I also give attribution. But I can also understand if such an option is not that desirable for you.

[MarkCiliaVincenti]

I think we can do that but I feel like it would still need to be in its own assembly due to https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock/blob/master/Backport.System.Threading.Lock/Properties/AssemblyInfo.cs

It would therefore more or less be a case of copying and pasting in the source files in a new class library.

[koszeggy]

That's why I acknowledged that I understand if you don't find my constraints appealing.

I see the following options:

1. Fork my repository, add your dependencies and use the changed code as you want. The changes will not be merged back to my repository because it would break my no dependencies rule.
2. Create a fork, add your changes with no dependencies, test everything and create a pull request. As I noted, if this basically means copy-pasting whole classes from your original code you can indicate the original source in comments. That's what I also ask when others re-use my code in a similar way, and that's what I also do as I noted in my previous comment.
3. No changes under the aforementioned conditions if you are not happy with them.

Side note: Just noticed the SecurityTransparent attribute in your AssemblyInfo.cs. Please note that such a dependency might not be fully compatible with my libraries, at least when targeting .NET Framework 4.x. I allow partially trusted callers for my library, and though I cannot apply SecurityRuleSet.Level2 to it due to a .NET Framework bug, I still apply the [SecurityCritical]/[SecuritySafeCritical] attributes wherever it's demanded by the CA2140, CA2141 rules (deprecated since .NET Core, affects .NET Framework only). See also the callers of the IsPartiallyTrustedDomain property and the unit test methods named *_PartiallyTrusted, like this one. These are very important to prevent the dreadful System.Security.VerificationException: Operation could destabilize the runtime error in various scenarios when the code is executed in a partially trusted domain (a real-life example is .NET Fiddle when targeting .NET Framework 4.7.2).

[MarkCiliaVincenti]

That's the issue. We don't want to put a TypeForwardedTo in your assembly either, and if we do this in its own assembly you're going to need to release it as its own nuget package, unless I'm missing something.

[koszeggy]

Now I checked why did you do that. When I backport something from the System namespace, I always do it as an internal type; otherwise, it would break the build at others who backport one of those types as well, or if they use a dependency that contains publicly backported types, such as one of the many compatibility packs. (I don't mean to say you should fix this, because this would be a breaking change for you now).

Also, I don't think [TypeForwardedTo] is really needed in your case. It's basically for the auto-forwarding mechanism of Type.GetType to resolve an assembly qualified type name with an old identity (mainly used by polymorphic serializers), and you cannot even apply the [TypeForwardedFrom] attribute to the "real" System.Treading.Lock type because you don't have access to it (not as if it would be required since .NET abandoned polymorphic serialization). If you don't have such a use case, it practically never happens that you have to resolve the old identity from a string, executed from the new version of your assembly.

And as you have the LockFactory class, your backported System.Threading.Lock doesn't need to be public either because you can publicly expose it by the factory anyway.

But as I mentioned, you can freely create a fork for private usage and add whatever is useful for you in any way.

[MarkCiliaVincenti]

There are two ways to use the library. One way for those on net5 or later is to use the System.Threading.Lock. The type forwarding is very important. Suppose my library is used by another library that targets net8 and net9. This library is then used by another library that targets only net8. Then finally a solution targeting net9 uses the latter library. What would happen is that it would not find the code it expects to find when it comes to locking.

This was how the library started but then there was the problem with thread aborts so I catered for it in a less elegant way, using the lock factory

[koszeggy]

Again, I will never backport a System type publicly because it can cause conflicts as noted above. Note that Tim also posted it as an internal type in his example.

What would happen is that it would not find the code it expects to find when it comes to locking.

It's because a public type is missing in a higher version of targeted platforms in your library, which exists in a lower version. ~You shouldn't do that.~ Edit: In my library you wouldn't need to do that because there wasn't a public System.Threading.Lock in the earlier versions that you need to forward.

(Anecdotical side note again: sometimes you cannot be prepared for similar conflicts with extension methods, i. e. when .NET introduces the same extension method that you already had. Even in this case you shouldn't remove your existing public method to keep binary compatibility but you can remove the this modifier to prevent build errors at the consumers of your library.)

If I ever will improve the existing locks in my library, I would add System.Threading.Lock internally only, which only makes the code simpler when targeting multiple platforms. It doesn't need to be public for pre-.NET 9 because it wouldn't mean a performance gain for older platforms anyway.

I cannot promise that I can do it for .NET 9 because my priority is one of my other libraries now so I would maybe improve my locks in a later version only. If the reason of your offer and willingness is that you use some locking types from my libraries, I still say that feel free to improve them, but if you add new dependencies or public backported System types, or I will not accept the PR. If you really want to add something like your LockFactory publicly, you can put it into the KGySoft.Threading namespace but TBH I don't have such plans. The internal backported Lock that Tim presented is simply small enough to add to every project internally that really requires it.

[MarkCiliaVincenti]

Since KGySoft.CoreLibraries is targeting pre-.NET 5.0, we cannot use the original backport.

Therefore if you're interested we could copy these two in: https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock/blob/master/Backport.System.Threading.Lock/LockFactory.cs https://github.com/MarkCiliaVincenti/Backport.System.Threading.Lock/blob/master/Backport.System.Threading.Lock/PreNet5Lock.cs

And then set them to internal.

We could then add something like this to the csproj:

<ItemGroup>
  <Using Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))" Alias="Lock" Include="System.Threading.Lock" />
  <Using Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))" Alias="Lock" Include="KGySoft.CoreLibraries.Backport.System.Threading.Lock" />
  <Using Alias="LockFactory" Include="KGySoft.CoreLibraries.Backport.System.Threading.LockFactory" />
</ItemGroup>

[koszeggy]

Therefore if you're interested we could copy these two in [...] We could then add something like this to the csproj

Actually I don't think that those are necessary. As I mentioned, there is no performance benefit in the backport for pre-.NET 9 at all so I simply would not add it to the library.

Instead, I would refactor the locks like this:

// the general change in classes with locker objects:
- private readonly object syncRoot = new object();
+ private readonly SyncObj syncRoot = SynchronizationHelper.CreateLock();

Where SyncObj could be a global alias, without backporting anything for older frameworks:

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

And the helper class could be as simple as this one:

internal static class SynchronizationHelper
{
    internal static SyncObj CreateLock() => new SyncObj();
}

The few explicit Monitor.Enter/Monitor.Exit calls of mine could be replaced by an additional couple of helper methods:

#if NET9_0_OR_GREATER
     internal static void Lock(SyncObj syncRoot) => syncRoot.Enter();
     internal static void Unlock(SyncObj syncRoot) => syncRoot.Exit();
#else
     internal static void Lock(SyncObj syncRoot) => Monitor.Enter(syncRoot);
     internal static void Unlock(SyncObj syncRoot) => Monitor.Exit(syncRoot);
#endif

And basically that's it. I don't use TryEnter in this project so I don't need that. And I have a couple of Monitor.PulseAll/Monitor.Wait calls that are not supported by the new locking alternative but it doesn't really matter as I need that for .NET Framework 3.5 only.

[MarkCiliaVincenti]

That would work, although you may want to aggressively inline there. Of course you're not enabling all the functionality in and have to use a different interface, but it could work.

I guess it's over to you then. Thanks for your insight and detailed explanations, learned a lot.