HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Class to manage updates coming in from another thread

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
managethreadanotherfromclasscomingupdates

Problem

Managing updates from other threads into Swing is a hard problem. In MVC design, if you don't want to have the Presenter be responsible for Thread safety, you can end up with deadlock issues, and also too many little tasks getting started; not great.

I have written a class designed to manage these updates in the view so that the Presenter is not coupled with the View's threading model. It loads them all into a queue, and then, if a new update is added, it processes the entire queue in case too many updates have come too quickly. Here's the class that I'm looking for feedback on.

Below, you will also find a harness that uses this class the way it's intended, to create a complete, working program. All feedback is welcome, but mostly I'm looking for thoughts on this SwingUpdater class. Note: this is Java 7 code, please no feedback that involves lambdas or method references.

import java.awt.EventQueue;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.atomic.AtomicBoolean;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public abstract class SwingUpdater implements Runnable {
    private static final Logger LOG =
            LoggerFactory.getLogger(SwingUpdater.class);

    private LinkedBlockingQueue updates;

    private AtomicBoolean updating = new AtomicBoolean(false);

    public SwingUpdater() {
        updates = new LinkedBlockingQueue<>();
    }

    public final void setObject(E object) {
        updates.add(object);
        if(!updating.getAndSet(true)) EventQueue.invokeLater(this);
    }

    @Override
    public void run() {
        while(updates.size() > 0) {
            try {
                doTask(updates.take());
            } catch (InterruptedException e) {
                LOG.warn("Interrupted while trying to process updates. Remaining updates: {}", updates.size());
                break;
            }
        }
        updating.set(false);
    }

    protected abstract void doTask(E update);
}


This

Solution

There's a minor race when the queue has just been emptied and the flag hasn't been reset yet. If a thread then adds an update it will not post the runnable to the EDT for running and the current invocation will just reset the flag and return.

This can be fixed by checking the queue again after resetting the flag.

size() on a linked list is slow. Prefer isEmpty instead. However queue has a method for taking out of the queue and returning a special value if it was empty:

@Override
public void run() {
    E e;
    while((e = updates.poll()) != null) {
        try {
            doTask(e);
        } catch (InterruptedException ex) {
            LOG.warn("Interrupted while trying to process updates. Remaining updates: {}", updates.size());
            //you should also log the exception itself
            break;
        }
    }
    updating.set(false);
}


(still has the race)

There is a danger for an infinite loop. If doTask calls setObject and handling that object also causes setObject to get called etc. then run will never return to the EDT.

You can fix this by using 2 queues; one for aggregation and one for handling the current set and swapping them when entering run().

This also fixes the race as when setObject is called while some are being handled then they get pushed to a new queue and the run gets scheduled again.

The run() callback should not be exposed to the calling code. Instead hide it in a nested class for that. In Java8 you can create a private function and pass this::handleAll to invokeLater().

Keep the updates queue as a Queue; you don't care exactly what it is only that it's thread safe.

setObject is kinda cryptic as a function name especially as it doesn't actually set any object. I suggest postUpdate instead.

Code Snippets

@Override
public void run() {
    E e;
    while((e = updates.poll()) != null) {
        try {
            doTask(e);
        } catch (InterruptedException ex) {
            LOG.warn("Interrupted while trying to process updates. Remaining updates: {}", updates.size());
            //you should also log the exception itself
            break;
        }
    }
    updating.set(false);
}

Context

StackExchange Code Review Q#102626, answer score: 4

Revisions (0)

No revisions yet.