HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Single-thread worker in multi-thread webservice

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
multiworkersinglewebservicethread

Problem

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 apiTasks = new Queue();
    static Thread thread;
    static AutoResetEvent wakeUp;

    public static void Init()    //  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(); // : 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.

Solution

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.

Context

StackExchange Code Review Q#152764, answer score: 3

Revisions (0)

No revisions yet.