1

I am writing a bot in Java for a game. One thread manages time for the Manager and Worker threads and sends heartbeats for them. Manager collects and interprets messages from the server, and the Worker threads receive commands from the Manager (with detailed information in classes) and acts on this information.

Right now, I'm having trouble with multithreading deadlocks and I'm not sure if there is an alternative.

Here are some methods in my manager class: I call getMessageFromUsername from the GUI thread when the user clicks a button. The following method is called from another thread.

private ArrayList<Message> MessageList = new ArrayList<>();

public synchronized Message getMessageFromUsername(String username) {        
        for( Message msg : MessageList ) {
            if( msg.username.equalsIgnoreCase(username) ) {
                Message m = new Message(msg.num, msg.username, msg.id);                
                return m;
            }
        }

        return null;
    }

My manager thread reads information from the socket and adds information to MessageList in a continuous loop. (Another one, i.e.)

private synchronized void parseMessage() {}
private void main() { while(1) { parseMessage(/*Adds message to MessageList*/); Sleep(); } }

Now, my problem is a pretty noobie issue- because if I want to write to MessageList I have to use synchronized to access it. However, since this happens in a loop and the messages are constantly coming in through the socket it causes a deadlock because it keeps locking my object and this occurs in a loop.

What can I do to solve my problem with deadlocking?

Jason
  • 1,297
  • 12
  • 24
  • The easiest way is to add some pauses in your loop. You can use `Thread.sleep()` – Balázs Édes Feb 19 '14 at 21:56
  • I'm using a 100 millisecond pause in my loop, but for some reason it still deadlocks. What is a good time to pause? – Jason Feb 19 '14 at 21:57
  • 3
    Wrong wrong wrong! @bali182 please say that you're trolling him! How on earth would a sleep be a proper solution to a deadlock? On the other hand, where exactly do you see a deadlock? What you have here is an example of a starving thread. `synchronized` primitive is not a solution for you, you should go for some prioritized mechanisms. – Piotr Zierhoffer Feb 19 '14 at 21:58
  • 1
    @bali182 That's terrible advice. This is exactly why answers should not be posted as comments, because comments can't be downvoted. – John Kugelman Feb 19 '14 at 22:02
  • 1
    @Jason Do you really mean _deadlock_ or are you really referring to _starvation_? Also, look at `java.util.concurrent.*` classes for a more granular approach to multithreaded collections. – Jim Garrison Feb 19 '14 at 22:02
  • isn't this the classic reader/writer problem? – Stijn Leenknegt Feb 19 '14 at 22:04
  • I don't think this is a pure reader/writer (consumer/producer) problem -- there does not seem to be a single consumer that just consumes from the stream. In particular, getMessageFromUsername looks like it is supposed to return immediately if there is no message. So wait() would not make much sense there. The problem is just a plain simple unnecessary lock while being blocked by blocking IO. – Stefan Haustein Feb 19 '14 at 22:23

4 Answers4

2

I think the main problem is that you seem to be keeping the lock while waiting for a message to arrive.

You lock via parseMessage() when you start reading a message -- which may not arrive for some time, and the other thread has no chance to do anything until a message actually arrives. And then it might just miss its chance... (wait / notify might solve the latter part of the problem, but I don't think it's needed here).

I'd restructure as follows:

 // just parse and return the message, don't add it to the list
 private Message parseMessage() {} 

 private void main() {
    while(1) {
       Message msg = parseMessage();
       synchronized(messageList) {
         messageList.add(msg);
       }
    } 
 }
Stefan Haustein
  • 18,427
  • 3
  • 36
  • 51
  • In my opinion, while it's very sensible to synchronize as little as possible and it's a bad idea to synchronize on any IO, it's worth mentioning that this doesn't *resolve* the problem of starvation. Just like sleep, it just moves the stress and (greatly) increases the chances for threads to interweave, but does not give any guarantees from theoretical point of view. It would probably help a lot in this case though ;-) – Piotr Zierhoffer Feb 20 '14 at 07:08
  • Where do you see the possibility of starvation in the changed program then? – Stefan Haustein Feb 20 '14 at 10:10
1

As I said in a comment, you don't have a deadlock, but you starve your thread.

You have two options:

  • either you go for some threads with prioritized ordering

  • or you go for a concurrency enabled collection. java.util.concurrent has many examples of such collections. But beware - modifying a collection that is foreached is never a good idea. Maybe you just need a kind of a fifo queue? One threads adds a message and another pops it and evaluates?

For example there is ConcurrentLinkedQueue, that provides Add method (to be used by your parseMessage) and isEmpty() (to be used in a while loop of your other thread).

While I'm not a Java expert, I'll point you to this answer: How to use ConcurrentLinkedQueue? It provides some practical advices.

Community
  • 1
  • 1
Piotr Zierhoffer
  • 5,005
  • 1
  • 38
  • 59
  • Thanks. I am looking into thread starving now. It looks like I was wrong about it being a deadlock. – Jason Feb 19 '14 at 22:05
  • General rule of thumb when assigning priorities to threads: Give threads that release a resource priority over threads that consume it. The thread that is adding messages to the queue is consuming a resource (memory). The thread that removes the messages from the queue and does something with them is (probably) releasing memory as it goes. – Solomon Slow Feb 19 '14 at 22:09
  • @jameslarge very true! But always have in mind to allow the other thread to do it's work, when the consumer is done ;-) – Piotr Zierhoffer Feb 19 '14 at 22:11
  • I don't see how priority locks would help here, but using concurrency enabled collections would probably avoid the problem. I think it still makes sense to exactly identify the problem though. – Stefan Haustein Feb 19 '14 at 22:25
  • @StefanHaustein Maybe the word "locks" wasn't the best in this context. But generally I meant the same thing as james large – Piotr Zierhoffer Feb 20 '14 at 06:56
1

If I am understanding your use case correctly then I believe that I'd look into using an implementation of BlockingQueue for your message list. As has already been mentioned there are also concurrent implementations provided such that no external synchronization is required on your part. You should be able to simply poll() or take() from the queue.

Ethan Anderson
  • 628
  • 1
  • 10
  • 20
0

I think you should use ArrayBlockingQueue and avoid using loops but replace loops with blocking methods like poll/take/put/offer.

Array based collections perform better and so you should consider first these then Linked node based collections. So ArrayBlockingQueue is better that than ConcurrentLinkedQueue.

Mak
  • 596
  • 5
  • 10