4
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ You may opt for tasks, instead of threads. To me your code look like a self rolled message bus on a db , you may consider msmq for the same purpose if possible. \$\endgroup\$ – pushpraj Jul 30 '14 at 8:34
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ What advantage is there for the ternary? It just makes the code harder to maintain, if any other statements needed to be in the "then" or "else" parts. Also, in SendTask(), you haven't really changed the scope. \$\endgroup\$ – Snowbody Mar 14 '15 at 2:07
  • \$\begingroup\$ @Snowbody In my version of SendTask(), you cannot use msg outside the loop. The ternary can make it more difficult to maintain, but that brings up the question of why should we ever use them; I feel this is as good a place for a ternary as any other. \$\endgroup\$ – user34073 Mar 14 '15 at 2:08
1
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$

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.