Making a task queue

Newbie & Debugging Questions
#1

In my current project, the game entities can be affected by the main game loop, the AWT thread, and later on, network packets or Lua scripts. To make things simple, I want to make a task queue to ensure that all of the actual changes happen in the main thread. This seems like it should work:


    private static final Vector<Runnable> tasks = new Vector<Runnable>(Settings.MAX_TASKS);

    public static void addTask(Runnable r)
    {
        synchronized(tasks)
        {
            tasks.add(r);
        }
    }
    private static void processTasks()
    {
        synchronized(tasks)
        {
            Iterator<Runnable> itr = tasks.iterator();
            while(itr.hasNext())
            {
                itr.next().run();
                itr.remove();
            }
        }
    }

It usually works OK, but if I get unlucky I get a ConcurrentModificationException in the processTasks() method when changing something using the GUI. Am I doing the synchronization wrong?

#2

ConcurrentModificationException doesn’t necessarily mean it’s been changed in another thread - could Runable.run() also be creating extra tasks and trying to insert them onto the queue while you’re iterating over it?

Alternatively you could take a copy of the list and iterate over that, or look into CopyOnWriteArrayList.

#3

Usually for any risky data collections I never access them directly. Instead, I create a synchronized accessor method for it.

It looks like you’re synchronizing everything correctly from the code you’ve shown, I merely do what I said for sanity’s sake.


public synchronized Vector<Runnable> getTasks()
{
    return tasks;
}

Then instead of tasks.add() you call getTasks().add(), etc.

Probably though your issue is what Orangy pointed out - you’ll get a ConcurrentModificationException if anything modifies tasks aside from that Iterator, so if run() can add a new task then that’ll break everything.

#4

Maybe I’m being dense, but I can’t see how that approach actually enforces anything? It looks like you’re still open to concurrent modification.

#5

Actually this doesn’t synchronize anything but the returning of the tasks field. As soon as you call any method on it, it is not synchronized anymore. So this code snippet doesn’t provide any improvement in a multithreaded environment.

You probably remove or add task in one of the tasks run() methods. So you change the tasks vector while you are iterating over it. This is a common problem in listener patterns and usually you just copy the vector to some temporary collection to iterate over it.

#6

Why use an Iterator full stop? It’s a queue! You only need thread safety when actually adding or removing each individual task.

ie. something like.


private static void processTasks() {
  while(true) {
    Runnable task;

    synchronized(tasks) {
       if (tasks.isEmpty()) {
          return;
       } else {
          task = tasks.get(0);
          tasks.remove(0);
       }
    }

    task.run();
 }
}

You’d be better looking at a different data structure (like a LinkedList) rather than Vector. Iif you’re Java 5+ then just look at ConcurrentLinkedQueue and you can remove all of your synchronization code - just loop through in your processTasks method until poll() returns null.

Best wishes,

Neil

#7

:o Uhduh… what was I thinking.

I’ve got to read up on synchronization and the right ways to do this, clearly. That means my current WIP has a blaring possibility for a concurrency crash.

#8

[quote]ConcurrentModificationException doesn’t necessarily mean it’s been changed in another thread - could Runable.run() also be creating extra tasks and trying to insert them onto the queue while you’re iterating over it?
[/quote]
That was the problem! It wasn’t supposed to be doing that, but in the future I might make a copy of the queue and iterate over the copy, rather than the original.