Advice

I am trying to ensure that methods in my class can only be called by a single thread. So far tried to use ReaderWriterLockSlim to achieve this effect but this can cause potential issues if it is not released correctly by the calling code. Issue is highlighted in code.

Simplified example to give the idea of the problem

public class Result
{
    //some code here
}
public static class Executor
{
    // here Start and Stop have to be called in sequence by the same thread.
    private static readonly ReaderWriterLockSlim _lock = new();

    public static void Start() {
        _lock.EnterWriteLock();
        try
        {
           // some code here
           // sets some internal variables and calls internal methods
        }
        catch (Exception)
        {
            _lock.ExitWriteLock();
            throw;
        }
    }

    public static Result Stop() {
        try
        {
            // some code here
            // sets some internal variables and calls internal methods

        }
        finally
        {
            _lock.ExitWriteLock();
        }
    }
}

// example caller
// calling class implementation is done by external user who does not know about Executor limitations

public static class Caller
{
    public static void Run(){

        // prepare parameters for Start()

        // it is important that only one thread can be used between Start() and Stop()
        Executor.Start();

        // code to use while Start() is running.

        // ISSUE: if code exits for any reason without calling Executor.Stop() it will never release lock.

        var result = Executor.Stop();

        // code to use after Stop() with result variable.
    }
}

// run multithreaded
class Program
{
    static void Main(string[] args)
    {
        var tasks = new List<Task>();
        tasks.Add(Task.Run(Caller.Run));
        tasks.Add(Task.Run(Caller.Run));
        tasks.Add(Task.Run(Caller.Run));
        Task.WaitAll(tasks.ToArray());
    }
}

Ideally would like to lock Executor class to a single thread not necessarily just methods. Any advise is much appreciated.

Edit: Both methods Start() and Stop() have to be called at very specific moments during execution.

Let me try and clarify the problem further.

  • The API requires to call methods Start() and Stop() at very specific points during execution. The code before Start() will prepare some parameters then after Start() the code will perform some manipulations and call internal parameters and finally Stop() will return a handle to an object that needs to be used later.
  • This is not something that can be changed due to complex internal structure of the code.
  • I think my description of threading problem is inadequate. To be more specific - methods Start() and Stop() have to be executed by the same thread. In other words if one thread called Start() then no other threads can call it until the original thread called Stop().
  • The above code using ReaderWriterLockSlim does solve that problem but is likely to cause thread locks if for some reason Stop() was not called.
Improve this question

13 Replies 13

Guru Stron
•

Without diving deep and at first glance - I would change Start to return IDisposable (which will do the necessary stuff in the dispose) and users can simply use the using instead of the Stop method.

Improve this answer
agracio
•

Unfortunately it is not that simple, Start() and Stop() methods have important logic and Stop() needs to be called at precise moment before the rest of the code is executed. The example is overly simplified so it is hard to show it. I will change it a little bit to emphasize it.

Improve this answer
Fildor
•

"// here Start and Stop have to be called in sequence by the same thread." - That's a problem. Clients will violate this and then complain. Make it easy to use your stuff correctly.

Improve this answer
agracio
•

Client will have very specific instruction on how to use the library but we cannot control how a user is approaching multithreading and need to lock it down.

Improve this answer
Fildor
•

Client will have very specific instruction on how to use the library

From experience: that's not going to work out. You'll be in service hell. Do yourself a big favor and make it impossible to use this API incorrectly especially if you have a hard set of specific requirements that must not be violated.

Improve this answer
agracio
•

Any library comes with instructions on how to use it, if you use it wrong way it will break. This is true for any library. However my concern is specific to multithreading.
Just to clarify - Start/Stop sequence is very specific to the library and is one of the most important things it does.

Improve this answer
JonasH
•

You generally want to design your API so that it gives compilation errors for as many problems as possible, and clear exceptions for any problems you absolutely have to check at runtime. Using documentation to specify correct usage should be the last resort since most people will not read the documentation.

In c# the general expectation is that static methods and properties are thread safe, since they would be very difficult to use otherwise. So I would not recommend making code like this static, making your Executor a regular non static class would make the dependencies much clearer, and allow you so simply lock on the object. You could maybe make it a singleton, but that is also something to be careful about.

I don't see why the using the Dispose-patter would be an issue here. But if it that critical to do things in a specific order it is even more important to design an easy to use API that helps the user do things in the correct order.

But if the core idea is to grant exclusive access to some resource it may be even better to use a task based or message passing pattern. Have a class with a method like Task<T> RunTaskWithResource(Func<MyResource, CancellationToken, T> method, CancellationToken cancel) that ensures only a single method is running at any one point in time. Usually by placing the task in a queue for execution. Either with just a regular Thread iterating over a BlockingQueue, or a custom taskScheduler, search for LimitedConcurrencyLevelTaskScheduler for examples. You can even make MyResource a ref-struct if you want to make sure that it cannot 'leak', but that may require a custom delegate with a allows ref struct generic constraint.

UI frameworks typically use a variant of this pattern. Often with some checks to detect incorrect usage, or "cross thread operation".

Improve this answer
Gerry Schmitz
•

This case comes up (a lot) for me when changing "context"; which requires, say, running queries and updating views. All kinds of events start firing when they shouldn't; e.g. "SelectionChanged" (usually to a "null" item). In this case, I use a "context-wide" "IsInitializing" flag. Anyone can set it; if it's off. Once it's on, it can only be turned off. While it's on, no one can enter those methods that say "IsInitializing" is on (except the "initializer"). It works well for this "event driven" case. Of course, no "thread" can be turning off when it never turned on in the first place.

Improve this answer
Mike Nakis
•

You are probably missing a _lock.EnterWriteLock(); between public static Result Stop() { and try, right?

Improve this answer
agracio
•

No it enters lock in Start() and releases in Stop(). It only works under ideal conditions, if anything was to happen between Start() and Stop() then lock would never release.
As described in the issue - both methods have to be executed by the same thread. If one thread calls Start() then no other threads can call Start() again until the first thread releases the lock by calling Stop().
Multiple threads cannot call Start()

Improve this answer
Mike Nakis
•

Okay then this is demented and cursed and whatever it is that you are trying to do, DON'T DO IT.

Improve this answer
Dmitrii Bychenko
•

If we want to lock, why do we use ReaderWriterLockSlim? Why not Lock? So if we want to implement Executor I suggest something like this:

public sealed class Executor : IDisposable {
  private readonly Lock _lock = new();

  public Executor() => _lock.Enter(); // Former "Start" method

  public void Dispose() => _lock.Exit(); // Former "Stop" method
}

and then

using (new Executor()) {
  ...
} // <- Dispose is guaranteed to be called here
Improve this answer
agracio
•

Thank you, I was thinking along the same lines. The only requirement for implementer would be to always have it inside `using` statement but we can manage that.

But one important change would be to make lock static, otherwise you will be creating a new Lock object for each instance.

private static readonly Lock _lock = new();
Improve this answer

Your Reply

By clicking “Post Your Reply”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.