0

I'll try to be brief. For a project of mine, I was tasked with developing a basic client-server program. The client has a basic GUI to work with and interact with the Server, the two interact via a middleman (Client Manager thread) and the Server dynamically creates Worker Manager threads tasked with creating and overseeing Worker threads.

The Client Manager creates Tasks based on the user's input on the GUI and sends them to the Server while also notifying the Worker Managers, who should be waiting by then. Once awake, Worker Managers will delegate tasks to their "minion" worker thread and the worker will take over. Worker Managers claim ownership over the Server's task list and take the first task for themselves (while also deleting the task from the Server's task list) and will then give it to their minion.

My issue, however, is that multiple workers are seemingly unable to read the full breadth of the task list. A single worker functions just fine so it appears to be a multi-threading/concurrency issue.

This is my Worker Manager's run method:

@Override
public void run() {
    try {
        while(true) {

            if(!server.getTaskList().isEmpty() && !minion.isWorking()) {
                 try {
                        synchronized(server.getTaskList()) {
                            while(!server.getTaskList().isEmpty() && currenttask == null) {
                            currenttask = server.getTaskList().get(0);
                            server.getTaskList().remove(0);
                            }
                        }

                        if(!minion.isAlive()) {
                            minion = new Worker(this);
                            synchronized(minion) {
                            minion.setCurrenttask(currenttask);
                            minion.start();
                            }
                        }
                        minion.setCurrenttask(currenttask);
                        synchronized(minion) {
                            minion.setAlarmclock(true);
                            minion.notify();
                            minion.setWorking(true);
                        }
                        synchronized(this) {
                            while(!alarmclock)
                                this.wait();
                                alarmclock= false;

                                currenttask = null;
                        }   
                    } catch (InterruptedException e) {
                        minion = new Worker(this);
                        minion.setCurrenttask(currenttask);
                        minion.start();
                    }
            }
            if(server.getTaskList().isEmpty())
                synchronized(this) {

                    while(!alarmclock)
                        this.wait();
                        alarmclock= false;
                    currenttask = null;
                }

        }
    } catch (InterruptedException e) {
        e.printStackTrace();
    }
}

And this my Worker run method:

@Override
public void run() {
    try {
        while(true) {

            if(currenttask != null) {
                System.out.println(this + " new task " + currenttask );
                if(manager.getServer().getTaskList().isEmpty())
                    System.out.println(this + "wtf ?");
                work(currenttask);
                isWorking = false;

            }
            else {
                synchronized(this) {

                    while(!alarmclock)
                        this.wait();
                        alarmclock= false;
                    }

            }
        }

    } catch (InterruptedException e) {
        e.printStackTrace();
    } finally {

    }
}

In my particular instance, I have 648 tasks at any given time but multiple workers/worker managers seem to "forget" about some of them, only processing 640 or 644 or 646 and then heading off to wait. I'm looking for anyone who may be able to point out if I made an obvious mistake here, given I'm not all that experience with concurrency and such.

TacoProgrammer
  • 1
  • What kind of object is your server's TaskList? – Lukas Bradley Dec 11 '17 at 19:03
  • I'm also worried about you using InterruptedException as a starting point for your new Minon. Take a look at this: https://stackoverflow.com/questions/2233561/producer-consumer-work-queues – Lukas Bradley Dec 11 '17 at 19:05
  • @LukasBradley It's an arraylist of Task objects I made. A task is a very simple class with only a few attributes: the ClientM that owns it and the String that comes with it plus some other basic data. The constructor for WorkerM already creates and runs its minion, I just use the InterruptedException in case it's ever interrupted. – TacoProgrammer Dec 11 '17 at 19:23
  • You are inconsistently accessing the same objects sometimes within a `synchronized` block, sometimes outside. That can’t work. And use local variables. How many times can you write `server.getTaskList()` within a tiny code fragment? By the way, `List.remove(int)` returns the removed element. There is no need to do `get(0)` followed by `remove(0)`… – Holger Dec 12 '17 at 17:08

1 Answers1

0

There is some key code missing in the example: what are the initial values of alarmclock and isWorking? Where is the initial minion created? Where is currentTask in the minion set to null after the work is done?

Secondly the way you've design the threading is really hard to follow (as you've discovered yourself), eg take this bit of code in the manager:

if(!minion.isAlive()) {
    minion = new Worker(this);
    synchronized(minion) {
        minion.setCurrenttask(currenttask);
        minion.start();
    }
}
minion.setCurrenttask(currenttask);

It's possible for the same task to be processed twice. When the minion thread starts, if it processes the task quickly then the second call to setCurrentTask looks like it will cause the minion thread to try and execute the same task again.

A third issue is that it looks like you can get a few "wtf ?" outputs. eg if you have a single task on the task list. A manger will be remove the task so the task list is empty. When you pass this task to the minion its going to complain that the task list is empty.

Fourthly you will be starting minions with the current task set to null. Two manager threads can both pass the first if statement if there is something in the task list. One of them will then get the task and the other will have null for the current task. You'll pass this null to the minion which seems like a waste of time.

Rather than trying to fix it, I'd recommend re-writing it. What it looks like you are trying to do is to take a task off a queue and pass it to a thread to be processed. I can't see any need to have more than one worker manager. This can read from the queue of tasks and pass a task to the next available minion.

If you don't mind introducing another library and doing a bit of reading, you can do this with Apache Camel fairly trivially. You'll end up with something like this.

If you don't want to learn Camel then learn about the high level concurrency objects in the java.util.concurrent package. For example look at using a BlockingQueue rather than ArrayList.

These high concepts were introduced specifically because trying to manually synchronise between threads is difficult and error prone.

matt helliwell
  • 2,574
  • 16
  • 24