5
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Nikita: Thanks for your reply. Altrough you're probably right about and thread-safety (I'll read about that), but: 1. I don't think the thread keeps running after closing. Not sure what actually happens, but application exits with code -1. Probably the thread crashes someway. But it's really easy to fix, so I will change that. 2. About racing condition: if, during execution of one task another ones will appear in queue, they will be also executed. The thread will start to wait only when the queue is empty. \$\endgroup\$ – Jakub Szułakiewicz Jan 17 '17 at 7:03
  • \$\begingroup\$ Bottomline: It's a shame, but it actually IS production code. Before changes the application crashes every hour, which makes it unusable. Now it crashes every second day, which is just annoying. Sometimes duct-tape solution is the only one, if it has to be make NOW. \$\endgroup\$ – Jakub Szułakiewicz Jan 17 '17 at 7:03
  • 1
    \$\begingroup\$ @JakubSzułakiewicz, 2) not during execution, but after the first task is completed, after you check whether or not the queue is empty (it is), but before you call Reset(). \$\endgroup\$ – Nikita B Jan 17 '17 at 7:26
  • 1
    \$\begingroup\$ @JakubSzułakiewicz, I couldn't disagree more with such attitude. If you need something now and you have no idea how to write it - you can always download an open-source implementation, there are plenty of those. Not to mention, that task queue is even implemented by Microsoft as part of WPF framework and you can safely reuse it: msdn.microsoft.com/en-us/library/… \$\endgroup\$ – Nikita B Jan 17 '17 at 7:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.