sentry-dotnet icon indicating copy to clipboard operation
sentry-dotnet copied to clipboard

Leverage new Lock types

Open jamescrosswell opened this issue 1 year ago • 2 comments

C# 13 includes a new Lock type intended to be used with the lock keyword.

Using Lock types as lock objects, the compiler can perform some optimisations that aren't possible when using standard object lock objects. It's a small thing, but we should probably replace the lock objects in our SDK to enable these optimisations.

jamescrosswell avatar Oct 16 '24 20:10 jamescrosswell

Polyfill v6.12.0 adds the System.Threading.Lock type. It satisfies the shape for the "C# 13 lock statement". The implementation uses System.Threading.Monitor under the hood.

I am not 100 % certain that this is 100 % correct, because there is a difference in IL-code-gen between the two lock-variants:

-object obj = new object();
-bool lockTaken = false;
+Lock @lock = new Lock();
+Lock.Scope scope = @lock.EnterScope();
try
{
- Monitor.Enter(obj, ref lockTaken);
  Console.WriteLine("mutual-exclusion");
}
finally
{
- if (lockTaken)
- {
-   Monitor.Exit(obj2);
- }
+ scope.Dispose();
}

where - with the polyfilled version on <= net8.0 - the implemented Monitor.Enter called via EnterScope would then be outside of the try-block.

Could this cause problems for our use cases?

Alternatively, we could #if NET9_0_OR_GREATER all usages of System.Threading.Lock, and use object instead, but that could bloat the source code quite a bit.

Alternatively, we could alias using Lock = object; globally (e.g. via C# or MSBuild) for #if !NET9_0_OR_GREATER, but this may (?) cause problems when we later on upgrade to a version of Polyfill that includes System.Threading.Lock.

(Just some thoughts for the implementer that occurred to me since I'm preparing for a presentation about C# language patterns and C# shenanigans (@EvaDitzelmueller)).

Flash0ver avatar May 12 '25 11:05 Flash0ver

Alternatively, we could #if NET9_0_OR_GREATER all usages of System.Threading.Lock, and use object instead, but that could bloat the source code quite a bit.

Yeah that's why we haven't done this already. That seemed like a pretty ugly solution (especially since we have no prospect of ever being able to drop net472 or netstandard).

I think the Polyfill implementation of Lock is the best option. The IL will never be the same as the net9.0 version but that's OK as long as we don't take a step backwards, performance wise, over what we have now.

jamescrosswell avatar May 12 '25 22:05 jamescrosswell

After a discussion

  • we're not sure about the Polyfill not causing potential issues on .NET Framework (or Runtimes that support Thread.Abort)
  • we could instead add a global using alias, to get the System.Threading.Monitor-based code for net8.0 and lower, but net9.0 and higher used the System.Threading.Lock-based code

e.g.

//GlobalUsings.cs
#if !NET9_0_OR_GREATER
global using Lock = object;
#endif

And then update usages

-private object _lock ...
+private Lock _lock ...

Flash0ver avatar Jul 16 '25 08:07 Flash0ver