SpinLock should be a class, not a struct

[mrpmorris]

When using a SpinLock, consuming code should be always referencing the same instance - as its purpose is to have shared state.

At the moment, there are at least two easy ways of accidentally breaking the functionality of a SpinLock on account that it is a struct rather than a class.

private readonly SpinLock Locker = new()

This causes a copy of the SpinLock to be created every time it is used. So Locker.Enter works on a new instance, and effectively disables any locking

Making Locker type members readonly is a common practice, but the caveat here is "unless it's a SpinLock".

Another example is as follows:

public static class SpinLockExtensions
{
  public static void ExecuteLocked(this SpinLock locker, Action action)
  {
    ...Get the lock
    ...Do stuff
    ...Release the lock
  }
}

This creates a copy of the SpinLock each time Locker.ExecuteLocked() is called. To fix this we have to change the code to this ref SpinLock locker.

It seems the idea of a single-instance with shared state contradicts the idea of a struct that is copied to a new instance.

Changing SpinLock to a class would allow readonly members (which is good practice) and avoid accidental copying on extension methods.

And, as far as I can see, there is no reason to ever want a SpinLock to be copied.

[svick]

private readonly SpinLock Locker = new()

Here is a proposal for that code to produce a warning: #33773.

Changing SpinLock to a class

That would be a breaking change, so I'm quite certain it's not going to happen.

[mrpmorris]

What would it break?

[svick]

If nothing else, it would be a binary breaking change. That is, it would break if you have a library that was compiled targeting a framework where it's a struct and try to run it without recompilation on a runtime where it's a class.

This is because the IL is different, e.g. calling Locker.Exit() looks like this for a struct:

ldarg.0
ldflda valuetype SpinLock C::Locker 
call instance void SpinLock::Exit()

But like this for a class:

ldarg.0
ldfld class SpinLock C::Locker 
callvirt instance void SpinLock::Exit()

The difference between ldflda and ldfld is what would cause the issue.

[mrpmorris]

I don't think .net 6 version assemblies will call .net 7 ones by default though, will they?

[svick]

@mrpmorris They will, if they are run on .Net 7.

Imagine I create a .Net 6 library that uses SpinLock and publish it to NuGet. Then you create a .Net 7 application and want to use my library. If SpinLock was changed from struct to class in .Net 7, then my library won't work in your application.

[mrpmorris]

In which case, mark it as obsolete and create a class named SpinLocker.

My intention is to give the gist of it, not the technical solution.

[richlander]

@stephentoub @terrajobst

[ghost]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When using a SpinLock, consuming code should be always referencing the same instance - as its purpose is to have shared state.

At the moment, there are at least two easy ways of accidentally breaking the functionality of a SpinLock on account that it is a struct rather than a class.

private readonly SpinLock Locker = new()

This causes a copy of the SpinLock to be created every time it is used. So Locker.Enter works on a new instance, and effectively disables any locking

Making Locker type members readonly is a common practice, but the caveat here is "unless it's a SpinLock".

Another example is as follows:

public static class SpinLockExtensions
{
  public static void ExecuteLocked(this SpinLock locker, Action action)
  {
    ...Get the lock
    ...Do stuff
    ...Release the lock
  }
}

This creates a copy of the SpinLock each time Locker.ExecuteLocked() is called. To fix this we have to change the code to this ref SpinLock locker.

It seems the idea of a single-instance with shared state contradicts the idea of a struct that is copied to a new instance.

Changing SpinLock to a class would allow readonly members (which is good practice) and avoid accidental copying on extension methods.

And, as far as I can see, there is no reason to ever want a SpinLock to be copied.

Author:	mrpmorris
Assignees:	-
Labels:	area-System.Threading
Milestone:	-

[terrajobst]

I’ll need to double check but making a value type a reference type is AFAIK a binary breaking change.

In terms of functionality I don’t think we’d want a class-based SpinLock because the idea is a low overhead type for leaf-level blocking:

Spin locks can be used for leaf-level locks where the object allocation implied by using a Monitor, in size or due to garbage collection pressure, is overly expensive.

There are already other class-level synchronization primitives that can be used instead of SpinLock. But I’ll let @stephentoub comment on this further.

[stephentoub]

What @terrajobst said. Monitor.Enter/Exit already works with any object and spins a bit as part of trying to enter, and should be the go-to way of protecting access to a region. SpinLock was made a struct on purpose, as an advanced type that can be used when avoiding the allocation of an object is important and the critical region being protected is expected to always be very brief. We're not going to change it to be a class (which would both be a breaking change and work against the goals of the type) nor obsolete it to replace it with an equivalent class-based version (if someone really wants a class-based spin-lock, SpinLock can just be embedded inside of any other object, including one dedicated to it). Thanks for the suggestion, though.

[mrpmorris]

@stephentoub I read in a book that a SpinLock is better than lock() for short term locks because lock will put a thread to sleep if it can't get the lock, is that no longer true?

[stephentoub]

a SpinLock is better than lock() for short term locks because lock will put a thread to sleep

lock has always spun a bit before falling back to a kernel-based blocking wait.

[mrpmorris]

I didn't know that, thanks!

[richlander]

What was the book?

[huoyaoyuan]

I read in a book that a SpinLock is better than lock() for short term locks

The "short term" may be much shorter than you thought. Look at our comment on SpinLock type:

    /// Spinning can be beneficial when locks are fine grained and large in number (for example, a
    /// lock per node in a linked list) as well as when lock hold times are always extremely short. In
    /// general, while holding a spin lock, one should avoid blocking, calling anything that itself may
    /// block, holding more than one spin lock at once, making dynamically dispatched calls (interface and
    /// virtuals), making statically dispatched calls into any code one doesn't own, or allocating memory.