-1

Given a scenario where there's a function that should only be executed by one thread at any given time, and the rest just return (since a specific state is already being worked on), what's the best way to accomplish this?

public void RunOnce()
{
    if(Interlocked.Exchange(ref m_isRunning, 1) == 1)
        return;

    // Run code that should only be executed once
    
    // What mechanism do we use here to ensure thread safety?
    Volatile.Write(ref m_isRunning, 0);
}

Would the same mechanism apply if m_isRunning is a state (ie. an integer representing an enum)?

Dennis19901
  • 615
  • 6
  • 9
  • Why are you trying to use a lockless implementation? Have you measured a performance problem with a solution using a proper synchronization primitive? – Servy Aug 02 '21 at 16:41
  • 1
    Writing lockless multi-threaded code is hard and hard to test (and fragile; maintenance programmers are known to occasionally delete essential (but seemingly useless) code from a working lockless implementation). C#'s `lock` mechanism is lightweight. Are you sure you need to roll-your-own? – Flydog57 Aug 02 '21 at 16:45
  • @Flydog57 A contending lock isn't lightweight at all. Also, if we just keep telling people to use "lock" because the alternatives are difficult, no-one is ever going to learn how to write performant multi-threaded applications. – Dennis19901 Aug 02 '21 at 17:02
  • @Dennis19901 But you're returning when the lock is taken out, which will be *real* fast. Again, have you actually measured the performance of this and ensured it's unacceptable for your uses? Odds are it'll be fine. And odds that you code a lockless version that actually behaves properly in all situations are *much* lower. Correctness before performance. – Servy Aug 02 '21 at 17:07
  • @Servy Lock contention is still going to happen. And you know what's also going to be real fast? Not using a blunt tool as a fix-all solution when it clearly isn't required for the use-case. It would be nice if more people on SOF could actually contribute to the question asked, so not only the person who asked, but also others viewing this question can learn. Rather than just pointing everyone to "this just works, use this". – Dennis19901 Aug 02 '21 at 17:14
  • @Dennis19901 Unnecessary lock contention would only happen if you wait for the lock to be free to continue executing, which you shouldn't do here. Skipping necessary synchronization may be (very marginally, in a way you probably can't even measure) faster, at the cost of not behaving properly and producing buggy code. I'm telling you to use the correct tool for the job at hand, rather than trying to use an improper tool precisely *because* anyone else coming to this question needs to hear that. – Servy Aug 02 '21 at 17:25
  • @Servy "An improper tool". I'm happy a lot of people who are actually well versed on multi-threading disagree with you there. And I'm also glad there are plenty of people on stackoverflow who don't just resort to a lock for every multi-threaded scenario. If you are not willing, or able to answer the question, which seems to be the case, I do not believe any further communication between us will yield any desired results. – Dennis19901 Aug 02 '21 at 17:47
  • @Dennis19901 Pretty much any expert on multithreading is going to tell you to only use lockless programming when you're extremely sure that more robust synchronization mechanisms aren't an option. Care to provide a source for that? Pretty much anyone on this site I've seen suggest a lock-free solution either wasn't experienced in writing multithreaded code, and frequently such solutions contained bugs, or knew that it was a situation where writing lock free code really was necessary in context (and even then, still lots of solutions with bugs). – Servy Aug 02 '21 at 17:55
  • 2
    Instead of `Volatile.Write` use `Interlocked.Exchange(ref m_isRunning, 0);` – Charlieface Aug 02 '21 at 18:01
  • I went looking to see if one of Eric Lippert's blog entries touched on Lockless programming. I didn't see one - that's a good warning sign. I searched for variations on "lockless c#" on the internet. I found some good articles (including Q&A here). Most repeat my "unless you really need to do this, stay away". You really need to understand the abstractions of the .NET memory model (barriers, etc.). Testing your code will be an absolute _bear_; it's likely to work properly nearly all the time, but not when you really need it in prod. @Charlieface's suggestion of using `Interlocked` is good. – Flydog57 Aug 03 '21 at 00:53
  • And... the `lock` keyword is some programmer candy around the `Monitor` class. You may find that `Monitor.TryEnter` does what you want: https://learn.microsoft.com/en-us/dotnet/api/system.threading.monitor.tryenter – Flydog57 Aug 03 '21 at 00:57

1 Answers1

1

The code in your question is thread-safe IMHO, but in general the Interlocked.CompareExchange method is more flexible than the Interlocked.Exchange for implementing lock-free multithreading. Here is how I would prefer to code the RunOnce method:

int _lock; // 0: not acquired, 1: acquired

public void RunOnce()
{
    bool lockTaken = Interlocked.CompareExchange(ref _lock, 1, 0) == 0;
    if (!lockTaken) return;
    try
    {
        // Run code that should be executed by one thread only.
    }
    finally
    {
        bool lockReleased = Interlocked.CompareExchange(ref _lock, 0, 1) == 1;
        if (!lockReleased)
            throw new InvalidOperationException("Could not release the lock.");
    }
}

My suggestion though would be to use the Monitor class:

object _locker = new();

public void RunOnce()
{
    bool lockTaken = Monitor.TryEnter(_locker);
    if (!lockTaken) return;
    try
    {
        // Run code that should be executed by one thread only.
    }
    finally { Monitor.Exit(_locker); }
}

...or the SemaphoreSlim class if you prefer to prevent reentrancy:

SemaphoreSlim _semaphore = new(1, 1);

public void RunOnce()
{
    bool lockTaken = _semaphore.Wait(0);
    if (!lockTaken) return;
    try
    {
        // Run code that should be executed by one thread only.
    }
    finally { _semaphore.Release(); }
}

It makes the intentions of your code cleaner IMHO.

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • @Servy I mean at any given moment, only one thread will be allowed to execute the code in the protected section. Do you think that the current abbreviated comment is misleading? – Theodor Zoulias Aug 03 '21 at 01:52
  • It's not misleading, it's just inaccurate. Any number of threads can be executing that code at the same time. The comment is true when using an actual synchronization mechanisms, as they provide such guarantees, but the lock free code *doesn't* provide that guarantee. That's exactly why so many people caution its use, the restrictions it provides on how the code can be reordered are *much* weaker. – Servy Aug 03 '21 at 01:59
  • @Servy could you provide a minimal example on [Fiddle](https://dotnetfiddle.net/) or anywhere else, proving that the `Interlocked.CompareExchange`-based implementation presented above allows concurrent execution of the protected region? – Theodor Zoulias Aug 03 '21 at 02:04
  • The whole point of multithreaded code is that you *can't* rely on any particular code having such a behavior, unless you're using tools that provide such a guarantee. It might work, or it might not. **And you can't know what it will do any one time you run it**. The operation tells you it's atomic. No more, no less. You're the one asserting that this is the answer. **You** show that the code reliably works in all situations and that it's not valid for the critical section to be observed to run from multiple threads at any one time. – Servy Aug 03 '21 at 02:19
  • 2
    @Servy I am sorry, I am not going to write an infinite number of programs in order to prove that my lock-free implementation guarantees exclusive execution of the protected region. What I am going to do is post this link: [Does Interlocked.CompareExchange use a memory barrier?](https://stackoverflow.com/questions/1581718/does-interlocked-compareexchange-use-a-memory-barrier), which covers quite nicely your code-reordering considerations IMHO. Finally I would like to thank you for sharing your knowledge with us. – Theodor Zoulias Aug 03 '21 at 02:28
  • Why would you require CompareExchange in the finally block? This code can only run once the code block has finished, successfully or not. Also, why would we require CompareExchange for the starting section? Exchange returns the previous value. So the only case where Exchange returns "not running", is when it previously was set to "not running". And lastly, why wouldn't Volatile.Write suffice? It makes sure operations are not reordered, and integer fields are atomic by nature. We are not interested in reading the value in the "finally" block, only setting it. – Dennis19901 Aug 03 '21 at 07:26
  • 1
    @Dennis19901 the reason for the `CompareExchange` in the finally block is the same reason that the `SemaphoreSlim` throws a `SemaphoreFullException` when it is released more times than it is acquired. It is for detecting software bugs. As for using `Interlocked.Exchange` and `Volatile.Write` instead of `CompareExchange`, now that I am thinking of it, in this specific case actually you can. The code in your question is thread-safe IMHO. – Theodor Zoulias Aug 03 '21 at 08:21
  • 1
    @TheodorZoulias Thanks. You make a good point about preventing the "release" multiple times. – Dennis19901 Aug 03 '21 at 08:26
  • @TheodorZoulias A statement that **some specific implementation* happen to ensure that this code will work **while others don't** makes it even more of a problem. That means you can write the code, test it, have it actually work in your testing environment, and then *not work in your production environment*. Why would you think quoting someone saying that it works *only on certain environments* supports your argument that this is actually an answer? – Servy Aug 03 '21 at 12:02