Long Thread & EDT

I’m trying to make a simple gui app that starts a “long” process that can be started and stopped. The process generates random numbers and displays them in a JList. As numbers are being displayed (i.e. process is started), user can select a number from JList and delete.
gui and uml http://mypages.valdosta.edu/dgibson/courses/cs4322/post.jpg

I'm interested to know what improvements I could make to my approach. (And yes, I need to read TrashGod's posts on EDT and SwingWorker, I will!)

```
public class MVC {
public static void main(String[] args) {
EventQueue.invokeLater(new Runnable() {
public void run() {
create();
}
});
}

private static void create() {
RandomNumberModel model = new RandomNumberModel();
Controller controller = new Controller(model);
View view = new View(controller);
controller.setView(view);

JFrame f = new JFrame("EDT & Long Thread");
f.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
f.add(view);
f.pack();
f.setLocationRelativeTo(null);
f.setVisible(true);
}
}

public class View extends JPanel {
JList list;
DefaultListModel listModel;
JTextArea area;
JTextField field;
JButton btnGenerate = new JButton("Start Generation");
JButton btnStop = new JButton("Stop Generation");
JButton btnDelete = new JButton("Delete selected");
Controller controller;

public View(Controller controller) {
this.controller = controller;
this.setSize(new Dimension(600, 300));

// Create GUI components
listModel = new DefaultListModel();
list = new JList(listModel);
area = new JTextArea();
btnStop.setEnabled(false);

btnGenerate.addActionListener(controller);
btnStop.addActionListener(controller);
btnDelete.addActionListener(controller);

// Assemble GUI components
JPanel buttonPanel = new JPan

Your code is well structured, and nice to read. This is a good thing.

You appear, from what I can see, to be using the appropriate threads for doing swing, and non-swing work. This is good.

There are a few thread-safe issues I can see:

-
Daemon Threads - where possible, you should use Daemon threads for background tasks. This makes the JVM shutdown easier, and is good practice for those times when you are not in a Swing environment (which does a 'hard' shutdown).

thread.setDaemon(true);
thread.start();

-
There is a possible race condition in the action-listener... there could conceptually be multiple action events queued up for the view.btnGenerate. These actions could be queued before the button is disabled. This would result in you having two generator threads running at once. I would recommend something like:

private final AtomicBoolean generatorRunning = new AtomicBoolean(false);

then, in your startNumGen thread I would have:

if (!generatorRunning.compareAndSet(false, true)) {
    // already running....
    return false;
}

this will gate the method explicitly... only one thread can start the generator (until it is stopped), and a thread can only start one of them (in case there are multiple queued events)...

-
additionally, I would replace the volatile isStopGen with this generatorRunning AtomicBoolean. Volatile is a complicated concept, and the AtomicBoolean does the same thing, but with better semantics....

you would replace the volatile isStopGen setting in the finally block with:

finally {
            view.btnGenerate.setEnabled(true);
            view.btnStop.setEnabled(false);
            generatorRunning.set(false);
        }

speaking of these semantics, you should check the generatorRunning.get() after the sleep, and before the set, rather than before the sleep. You have the risk here that you stop the generator, and then it still generates a value half a second later.... Your code could look like:

private void doGeneration() throws StopGenException, InterruptedException {
    for(int i=0; i<10; i++) {
        final double val = model.getRandomNum();

        // check after sleep... 
        if( !generatorRunning.get()) {
            throw new StopGenException();
        }
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                // potential double-check in a slow-to-schedule thread....
                if (generatorRunning.get()) {
                    view.listModel.addElement(val);
                }
            }
        });
    }
 }

StackExchange Code Review Q#42089, answer score: 4