Database threading design

Question

I am new to threading and I am a junior developer, so I guess there are many mistakes. My scenario is this:

• look into the database
• if there are data which should be sent
  • add the data to the queue
• if queue is not empty
  • send the next message and remove it from the queue
  • sleep 10 seconds
• if a message from another thread arrived
  • stop waiting and go to next message in queue
• if no message from another thread arrived
  • go back to the first step, but do this only 2 times
  • if a message still hasn't arrived from another thread
    • pass to the next message until queue is empty
• return to the first step

I am trying to do this like this:

private Thread ReceiveThread;
private Thread SendThread;
internal static Thread ServiceThread;

These 3 threads:

ReceiveThread = new Thread(ReceiveTask);
ReceiveThread.Start();
ServiceThread = new Thread(SerAutoThread.SendServiceMsg);
ServiceThread.Start();
SendThread = new Thread(SendTask);
SendThread.Start();

class SerAutoThread
{
    internal static object[] NextService;
    public static readonly object _locker = new object();
    internal static Queue<object[]> Services;
    internal static int sendingTime = 0;
    private static DatabaseFirebird DB;
    internal static void SendServiceMsg()
    {
        DB = new DatabaseFirebird();
        DB.Open(ConnectionStr);
        Services = new Queue<object[]>();
        while (true)
        {
            if (Services.Count != 0)
            {
                SetNextSerAndSend();
            }
            else
            {
                CheckAndSetServices();
            }
        }
    }


    private static void SetNextSerAndSend()
    {
        NextService = Services.Dequeue();
        for (int j = 0; j < 4; j++)
        {
            if (sendingTime == TRANSMITTED)
            {
               //pass to next msg
               sendingTime = 0;
               j = NEXTMSG;
            }
            else if (sendingTime < 3)
            {
                sendingTime++;
                Byte[] data = SetNextPckage();
                DeviceManager.MessageSendQueue.PostItem(new SendMessage("UDPCmd",
                NextService[(int)NextMsg.DeviceId].ToString(), 
                data, data.Length));
                MyDebug.WriteLine("Sended...");
                lock (_locker)
                {
                    Monitor.Wait(_locker, TimeSpan.FromSeconds(10));
                }
            }
            else 
            {
                // pass to next msg
                j = NEXTMSG;
            }
        }
    }
}

private void ReceiveTask()
{
    ReceiveMessage receiveMsg;
    while (true)
    {
        receiveMsg = Com.MessageReceiveQueue.GetItem(-1);
        SerAutoThread.sendingTime = SerAutoThread.TRANSMITTED;
        lock (SerAutoThread._locker)
        {
            Monitor.Pulse(SerAutoThread._locker);
        }
    }
}

 private void SendTask()
 {
     SendMessage msg;
     while (true)
     {
         msg = MessageSendQueue.GetItem(-1);
         String rtrn = PushData(msg);
     }
 }

Is this thread-safe or not? I'm not sure if something is wrong with the design or I'm doing something wrong elsewhere.

And this is not my entire code (for understanding). When I actually run the app, this works fine. Yesterday just ones my Database management system (Firebird) locked. Then I stopped my app, stopped Firebird server then started again and ran my app again. Now it is ok again. I am not using threads with deep understanding. So I am worried about thread safety.

Is it possible for the thread-locked Firebird?

Answer 1

This looks pretty good to me. I am not that good at threads myself, so I won't comment on the thread safety, but these are a few things I noticed.

This is fine as it is, but you might want to consider making this a ternary:

if (Services.Count != 0)
{
    SetNextSerAndSend();
}
else
{
    CheckAndSetServices();
}

Here it is in ternary form:

Services.Count == 0 ? CheckAndSetServices() : SetNextSerAndSend();

I'm not sure how this would affect the performance of your code, or whether it is important that the variable stay the same, but it is good to always keep your variables in as tight a scope as possible:

private void SendTask()
{
    SendMessage msg;
    while (true)
    {
        msg = MessageSendQueue.GetItem(-1);
        String rtrn = PushData(msg);
    }
}

This could possibly become:

private void SendTask()
{
    while (true)
    {
        SendMessage msg = MessageSendQueue.GetItem(-1);
        String rtrn = PushData(msg);
    }
}

The same principle can be applied to ReceiveTask.

Answer 2

You have a classic situation where multiple threads can access the database and two operations (a read and an update) are required to do the work.

It is possible to make such schemes work, but it is hard work - you will need a transaction around the read/update operation and your code will possibly need to handle deadlock exceptions. Debugging and testing the deadlock code is difficult, as it will seldom happen.

If only one application is involved, then I suggest you delegate database operations to a singleton thread-safe class. Synchronizing its methods will ensure that only one thread accesses the database at any time.

If you need the database to do implement thread safety, consider setting the transaction isolation level to SERIALIZED. This should fix the problem at the expense of performance.