5
\$\begingroup\$

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<E> implements Runnable {
    private static final Logger LOG =
            LoggerFactory.getLogger(SwingUpdater.class);

    private LinkedBlockingQueue<E> 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 class uses SwingUpdater as it's intended to be used; I'm sure this class could be better written (and more than one class) but I threw it together quite quickly to be able to provide a complete working program with a main method and so forth for SwingUpdater.

Here it is:

import java.awt.*;
import java.util.Random;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

import javax.swing.*;

public class SwingUpdaterExample {
    private final JFrame testFrame;
    private final JPanel contentPane;
    private final JTextPane textPane;
    private int updateCount;

    private final SwingUpdater<String> updater = new SwingUpdater<String>() {
        @Override
        protected void doTask(String update) {
            String oldText = textPane.getText();
            textPane.setText(oldText + ((updateCount & 3) == 0 ? System.lineSeparator() : "\t") + update);
            updateCount++;
        }
    };

    public SwingUpdaterExample() {
        textPane = new JTextPane();

        contentPane = new JPanel(new BorderLayout());
        contentPane.add(textPane);

        testFrame = new JFrame("ExampleFrame");
        testFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        testFrame.setSize(500,800);
        testFrame.setContentPane(contentPane);
    }

    public void setVisible(boolean visible) {
        testFrame.setVisible(true);
    }

    public void appendText(String string) {
        updater.setObject(string);
    }

    public static class ExampleTask implements Runnable {
        private final String name;
        private final SwingUpdaterExample view;

        public ExampleTask(String name, SwingUpdaterExample view) {
            this.name = name;
            this.view = view;
        }

        @Override
        public void run() {
            Random r = new Random();
            for(int counter = 0; counter < 15; counter++) {
                if(r.nextDouble() < 0.3) {
                    try {
                        Thread.sleep(2000 + r.nextInt(4000));
                    } catch (InterruptedException e) {
                        break;
                    }
                }
                view.appendText(name + " - " + counter);    
            }
        }
    }

    public static void main(String[] args) throws Exception {
        final SwingUpdaterExample frame = new SwingUpdaterExample();
        EventQueue.invokeLater(new Runnable() {
            @Override
            public void run() {
                frame.setVisible(true);
            }

        });

        ExecutorService service = Executors.newCachedThreadPool();
        for(int i = 0; i < 4; i++) {
            ExampleTask task = new ExampleTask("Task" + i, frame);
            service.submit(task);
        }

        service.shutdown();
    }
}
\$\endgroup\$
4
\$\begingroup\$

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<E>; 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.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Doesn't LinkedBlockingQueue cache the value of size? \$\endgroup\$ – durron597 Sep 2 '15 at 20:39
  • \$\begingroup\$ Also fwiw this is Java 7 code. \$\endgroup\$ – durron597 Sep 2 '15 at 20:40
2
\$\begingroup\$

Overall I feel the concept is unnecessary in a disciplined environment, but I agree that asserting a different protocol for applications may be useful. Note that there are a couple of drawbacks to your approach:

  1. one of the guarantees is that the Swing EDT will process events in the order they are received. Your solution will not, though, it will batch all events of the same type together. This may lead to a number of visual distractions as events happen out of order between different event types, for example.
  2. this is not preventing people from using the existing model anyway, it is adding another standard.

Having said that, if these issues are OK to live with, there are still a number of technical issues with your implementation....

Hygiene

The following thoughts are what I have based on your current implementation. Things I would fix even if I did not change the way the code works.

Your initialization of your class is inconsistent and overly verbose. Here's the relevant code:

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

    private LinkedBlockingQueue<E> updates;

    private AtomicBoolean updating = new AtomicBoolean(false);

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

The Logger is fine, but the updates and updating should be final too. Additionally, there is no need to put the updates initializer in to a constructor, it can be constructed at declare time. Also, it can be just the interface BlockingQueue that's used, not the concrete class LinkedBlockingQueue. That removes the need for the constructor entirely.

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

    private final BlockingQueue<E> updates = new LinkedBlockingQueue<>();
    private final AtomicBoolean updating = new AtomicBoolean(false);

A class called SwingUpdater should have a primary method called update(...) or something. It took me a moment to discover that setObject(...) is the primary method. That's doubly confusing because it is a hungarian-style name, (setting an Object), but it is not an Object you pass in, instead it's a E. I like how you have made the method final, though. I would instead have something like:

public final void update(E input) {

Now, I know this is a style thing, but this line is unconventional for almost all style guides I am aware of:

if(!updating.getAndSet(true)) EventQueue.invokeLater(this);

Yeah, yeah, I know, but even for non-braced 1-statement conditions, at least use a new line, and indentation:

if(!updating.getAndSet(true))
    EventQueue.invokeLater(this);

Preferably brace it too (like style guides recommend - they also recommend a space after the if, and before the ():

if (!updating.getAndSet(true)) {
    EventQueue.invokeLater(this);
}

While you were clear that the update (setObject) method is final, and can't be overridden, you have not made the run method final. This allows overriding classes to alter the way the events are handled by the queue. That's a bad thing. The run should be final.

Your abstract method looks fine, but I would synchronize the parameter name with the update (setObject) method. Currently your parameter is update but I would call it input... so it would be:

protected abstract void doTask(E input);

Defensive Programming

I am concerned about two things in your code. The non-final run method alerted me to the fact that people can override its behaviour, but it's worse than that.... people who override your class can easily call the run method from the wrong thread.

You need to ensure that the run method is in the right context before you create thread conflicts in ways you are supposed to be preventing! A simple:

if (!SwingUtilities.isEventDispatchThread()) {
    throw new IllegalStateException("Cannot update components unless running on the EDT");
}

would be sufficient to catch those people who inadvertently misunderstand the way your Runnable is supposed to work.

The second problem I see in your code is the poor handling of the InterruptedException. At a minimum, you should re-set the interrupted flag when you log the message. Even if you don't know how to handle the exception, you should let someone who can handle it to do it.

        } catch (InterruptedException e) {
            LOG.warn("Interrupted while trying to process updates. Remaining updates: {}", updates.size());
            Thread.currentThread().interrupt().
            break;
        }

A better approach

I am a bit concerned about the small window race condition as well. I see cases where events may get delayed without processing until a subsequent trigger happens. A better way would be to reverse the order of the reset of the flag. If you reset it before emptying the queue, then the result of a race condition will be to process an empty queue, instead of not processing an occupied queue.

But, having played with it, I think there's a better way to handle the race condition by using the drainTo function of the queue. This also eliminates a blocking operation, so there's no need to handle the InterruptedException at all. Consider the following implementation:

public abstract class SwingUpdater<E> implements Runnable {

    private final BlockingQueue<E> updates = new LinkedBlockingQueue<>();
    private final AtomicBoolean updating = new AtomicBoolean(false);

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

    @Override
    public final void run() {
        if (!SwingUtilities.isEventDispatchThread()) {
            throw new IllegalStateException("Not on the EDT");
        }

        updating.set(false);

        Deque<E> pending = new LinkedList<>();
        updates.drainTo(pending);

        while (!pending.isEmpty()) {
            doTask(pending.removeFirst());
        }

    }

    protected abstract void doTask(E input);
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Great answer! I think the main thing this answer is missing that Updater should probably not implement Runnable, there should be a private inner class to keep run encapsulated, as ratchet freak suggests in the other answer. This would also prevent the need for checking whether run is called on the EDT. \$\endgroup\$ – durron597 Sep 3 '15 at 3:51

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.