-
Notifications
You must be signed in to change notification settings - Fork 930
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Encourage use of .NET 9.0's System.Threading.Lock #3601
base: master
Are you sure you want to change the base?
Encourage use of .NET 9.0's System.Threading.Lock #3601
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the proposal,
bUnit-dev/bUnit#1532 has a discussion about this which is relevant for NHibernate too.
For the simple usages we currently have (excepted the Monitor one which would have to be slightly different), using something like:
#if NET_9_0_OR_GREATER
private readonly Lock syncLock = new();
#else
private readonly object syncLock = new();
#endif
Seems enough and would avoid adding a new dependency for users to investigate if they want it in their project or not.
@@ -6,7 +6,7 @@ | |||
<VersionPatch Condition="'$(VersionPatch)' == ''">0</VersionPatch> | |||
<!-- Clear VersionSuffix for making release and set it to dev for making development builds --> | |||
<VersionSuffix Condition="'$(VersionSuffix)' == ''">dev</VersionSuffix> | |||
<LangVersion Condition="'$(MSBuildProjectExtension)' != '.vbproj'">12.0</LangVersion> | |||
<LangVersion Condition="'$(MSBuildProjectExtension)' != '.vbproj'">preview</LangVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, it needs language version 14.0? We will likely wait for it before merging your proposal, if we accept it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not spotted what does require such a change. Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's 13.0 as such, and comes with .NET 9.0, but for now you need to set the LangVersion to preview on .NET 9.0 in order to lock on a System.Threading.Lock object. The expectation is that this changes as soon as .NET 9.0 is released.
@@ -73,6 +73,10 @@ | |||
<PackageReference Include="Microsoft.CSharp" Version="4.7.0" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))"> | |||
<PackageReference Include="Backport.System.Threading.Lock" Version="1.1.6" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, that is this Nuget package, coming from your repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct.
Yes, it is very relevant. Using the code you pasted above will give "CS9216: A value of type System.Threading.Lock converted to a different type will use likely unintended monitor-based locking in lock statement." when you try using it in |
As for your concern regarding the additional dependency, I understand, but what we can do is copy the code into NHibernate and just put in a comment at the top giving credit. It's not ideal, but if the dependency is a deal-breaker for you we can do that instead. |
Thanks for the contribution. It is unlikely we'll be targeting .NET 9 version as it is not LTS. Also, we would like to avoid an external dependency. |
We could copy the code in. But if you don't intend on targeting .NET 9 that's another story because this would have to wait until .NET 10. |
@fredericDelaporte a new version of Backport.System.Threading.Lock has come out that acts as a source generator and basically dropping the DLL as a dependency. If this new development makes you reconsider, I will amend this PR accordingly. |
Will improve performance once the library starts targeting .NET 9.0 as locking on System.Threading.Lock is ~25% more performant over locking on an object. Newly-added micro dependency allows backported use without affecting performance.