Single-thread worker in multi-thread webservice

Question

There is standalone WCF webservice, which is providing methods to interact with legacy ERP system.

This system exposes an API, which allow programmer to interact with it.

The problem is that, for legacy reasons, every call to API function has to come from the same thread. As we know, WCF webservice is multi-thread, so I needed to solve it someway.

Below is my solution of that problem. It works. I want to someone smarter than me look at this code, and tell me, if that is "right" way to solve it. Maybe there was "better" way to achieve goal. I want to learn something.

public static class ApiManager
{
    static Queue<ApiTask> apiTasks = new Queue<ApiTask>();
    static Thread thread;
    static AutoResetEvent wakeUp;

    public static void Init()    // <--1
    {
        wakeUp = new AutoResetEvent(false);
        thread = new Thread(runner);
        thread.Start();
    }

    public static void AddTask(ApiTask task) //<--5
    {
        apiTasks.Enqueue(task);
        wakeUp.Set();
    }

    static void runner()
    {
        wakeUp.Reset();
        while (true)  //<--2
        {
            wakeUp.WaitOne();      //<--3
            while (apiTasks.Count > 0)
            {
                var task = apiTasks.Dequeue(); //<--4
                task.Start();
            }
            wakeUp.Reset();
        }
    }
}

This is my "worker" class. (1) Init method - it is run on start of webservice. It creates new thread, and starts it. In that thread we have infinite loop, in which thread is waiting for wakeUp signal to be set (3). After that, it consumes elements from queue (4), and runs it one after another. After the queue is empty, it resets the wakeUp signal, and waiting for it to be set.

public abstract class ApiTask
{
    protected abstract void DoJob(); //<-- 1
    protected AutoResetEvent IsDone = new AutoResetEvent(false);
    protected Exception RegisteredException = null;

    internal void Run()
    {
    }

    public void Start() 
    {
        try
        {
            DoJob();
        }
        catch (Exception ex)
        {
            this.RegisteredException = ex;
        }

        IsDone.Set();
    }
}

public abstract class ApiTask<TResult> : ApiTask where TResult : new()
{
    protected TResult Result = new TResult();

    public TResult GetResult()
    {
        ApiManager.AddTask(this);
        this.IsDone.WaitOne();

        if (RegisteredException != null) throw RegisteredException;
        return Result;
    }
}

Here we have abstract class ApiTask, which is stored in queue in ApiManager, and run by it. It contains abstract method DoJob (1), which will actually contains code to be run. Also, we have Start method, which runs the job, and sets IsDone signal, after finishing. If job throws an exception, it is catched and stored. And - the "core": GetResult method - it adds task to queue in manager, wait for it to be done, and returns result (or throws exception, if it has been stored by Start method).

In practice, I'm creating Tasks in that way:

public abstract class CreateDocumentTask:ApiTask<APIDocument>
{
    DateTime _date;

    protected int CreateDocumentTask(DateTime date)
    {
        _date = date;
    }

    protected override void DoJob()
    {
        Result = APICREATEDOCUMENTFUNCTION(_date);
    }
}

To use it in application, I only have to change call from:

var document = APICREATEDOCUMENTFUNCTION(date)

to:

var document = new CreateDocumentTask(DateTime.Now).GetResult();

And I'm sure that it will be always done by the same thread.

Feel free to correct my English. Feel free to correct and criticize code. I want to learn.

Answer 1

Basically, you have the right idea: producer-consumer is probably the pattern you need.

The way you implemented it is the problem, because so many things have gone wrong...

• Queue is not thread-safe, so access to it has to be synchronized, otherwise at best you gonna be losing tasks at worst it will straight up crash your software.
• I don't see a single good reason why your ApiManager should be static. Having a mutable static dependency in code is already bad enough, having a static dependency that you also have to synchronize - is straight up awful.
• Your thread is never terminated and it will keep running even if you close your application.
• You have racing conditions all over the place. For example, if you have only two tasks, and the second one is added to queue after the first one is in completed but before wakeUp.Reset() is called in your loop, the second task will never be executed.
• You create IDisposable objects that you never dispose (I'm talking about wait handles).

Bottom line is: it's hard to fully understand thread-safety and it's even harder to write correct thread-safe code in complex multi-threaded environment. It is not something you can just jump into. You should definitely do some reading on how multi-threading is done in C# before attempting to implement it in your production code. Reading lock keyword documentation is a good place to start.