If Swing models' getters aren't thread-safe, how do you handle them?

It is well known that updating a Swing GUI must be done exclusively in the EDT. Less is advertised that reading stuff from the GUI must/should also be done in the EDT. For instance, let's take ButtonModel's isSelected() method, which tells (for instance) ToggleButton's state ("down" or "up").

In every example I've seen, isSelected() is liberally queried from the main or whichever thread. But when I look at DefaultButtonModel's implementation, it's not synchronized, and the value is not volatile. So, strictly speaking, isSelected() could return garbage if it's read from any other thread than the one from which it's set (which is the EDT, when the user pushes the button). Or am I mistaken?

I originally thought about this when shocked by item #66 in Bloch's Effective Java, this example:

public class StopThread {
    private static boolean stopRequested;

    public static void main(String[] args) throws InterruptedException {
        Thread backgroundThread = new Thread(开发者_StackOverflow中文版new Runnable() {
            public void run() {
                int i = 0;
                while(!stopRequested) i++;
            }
        });
        backgroundThread.start();

        TimeUnit.SECONDS.sleep(1);
        stopRequested = true;
    }
}

Contrary to what is seems, that program never terminates, on some machines at least. Updating the stopRequested flag from the main thread is invisible to the background thread. The situation can be fixed with synchronized getters & setters, or by setting the flag volatile.

So:

1. Is querying a Swing model's state outside the EDT (strictly speaking) wrong?
2. If not, how come?
3. If yes, how do you handle it? By luck, or by some clever workaround? InvokeAndWait?

1. No, not wrong, but as with any cross-thread communication you need to ensure you provide appropriate thread-safety mechanisms if you decide to do this (e.g. use of synchronized or volatile). For example I typically write my own TableModel implementations, typically sitting on List<X> where X is my business object. If I intend for other threads to query the List I will make this a synchronized Collection. It's also worth noting I would never normally update the List from other threads; only query it.
2. Because it's exactly the same situation as with any other multi-threaded application.
3. I typically make the collection synchronized (see 1).

Caveat

Despite my answer above I typically do not access models directly outside of the EDT. As Carl mentions, if the action of performing an update is invoked via some GUI action there's no need to perform any synchronization as the code is already being run by the EDT. However, if I wish to perform some background processing that will lead to the model being changed I will typically invoke a SwingWorker and then assign the results of doInBackground() from within the done() method (i.e. on the EDT). This is cleaner IMHO as the doInBackground() method has no side-effects.

Yes it is wrong; unless both threads synchronize on the same object or use some other memory barrier, like volatile, as defined by the JMM one or the other can observe inconsistent memory contents. Period. End of story. Violating this might work on some, even many, architectures, but it will eventually bite you on the hiney.

The problem here is that with a few exceptions where you provide the model, such as what Adamski referred to, the Swing code is not going to synchronize on anything.

And it's worth noting that the JMM (Java Memory Model) has changed with JSR-133 in Java 5 in very important ways, so behavior with a J4 and earlier JVM may well yield different results than J5 and later. The revised JMM is, as you would expect, considerably improved.

Swing is in general not only not thread-safe, but thread-hostile. However, most models, other than Swing text, are thread-agnostic. This means that you can use models in any thread, provided you use standard thread protection. Swing text was an attempt to be thread-safe but failed, and is in fact thread-hostile. Only use Documents from the Event Dispatch Thread (EDT).

The usual advice is to run potentially long running (typically blocking) tasks off the EDT. However, threading is difficult. SwingWorker is a popular way to induce bad design - avoid it. Try to keep EDT and non-EDT code separated. Try not to share mutable state.

Threading bugs are difficult. You might not see them on your machine, but customers may well do. Perhaps bugs appear due to optimisations in later versions of the JRE. Thread bugs are difficult to track. Therefore, be conservative. Even blocking the EDT briefly is better than nasty bugs.

1. I never worried much about it, but strictly speaking, yes, it's probably wrong.
2. N/A.
3. I let changes in the GUI update (via listeners) models of my own construction, that are not contained or constructed by Swing. There's never any need to ask the GUI because the GUI updates the model.

Since I create the models myself (this can be something very simple, such as a String with getter/setter and PropertyChangeSupport), I can make the accessors synchronized if I want. I rarely encounter flaky behavior due to thread contention or such.

If yes, how do you handle it? By luck, or by some clever workaround? InvokeAndWait?

Others have already mentioned SwingWorker and you're already aware of the invoke* methods. javax.swing.Timer can also be useful. There are no silver bullets for safe multi-threaded programming, though a good design will go a long way.

There are things you can do to detect bugs. If you design a method to only be invoked from the event dispatch thread, guard it so that it will fail if someone tries to access if from another thread:

  public static class ThreadUnsafeAccessException extends RuntimeException {
    private static final long serialVersionUID = 1L;
  }

  @Override public Object getElementAt(int index) {
    if (!SwingUtilities.isEventDispatchThread()) {
      throw new ThreadUnsafeAccessException();
    }
    return decorated.getElementAt(index);
  }

Retrofitting this approach to an existing application that does a lot of setup on the main application thread might mean some major refactoring.

This is an excellent question and Software Monkey has the right answer. If you want to do effective threading you must fully understand the Java Memory Model (JMM). Note that the java memory model has changed with JSR-133, so a lot of old threading examples you find will simply be wrong. E.g., the volatile keyword has changed from being almost useless to now being nearly as enforced as the synchronized keyword.

Additionally, we now have true, ubiquitous multi-core cpu's, which means true multi-threading. Multi-threading up until a few years ago was only faking it on single-core cpu's, which is why so much bad sample code is out there. It used to work. Now it won't.

Seriously, if Joshua Bloch can get it wrong, tread very carefully. It is complicated stuff (and yes, his code is wrong. Make that variable volatile and it will work in all cases though).

Oh yeah, Adamski has it right. Use a SwingWorker to make sure long-running code is done in a background thread and that when it is done manipulating the data, it should access Swing on the done() method. If you need to read, do it before making your SwingWorker and make the info final.

Example of code that would be called on the EDT, most likely in an ActionListener or somesuch (psuedocode):

public void someOverlySimpleExampleThatIsCalledOnEDT() {
   /*
    * Any code run here is guaranteed to happen-before 
    * the start of a background thread.
    */
   final String text = myTextField().getText();
   SwingWorker sw = new SwingWorker() {
      private volatile List data;
      public Object doInBackground() {
         //happens in background thread. No Swing access
         data = myDao.getDataFromInternet(text);
         return null;
      }
      public void done() {
         //Happens in EDT. Swing away Merrill
         loadDataIntoJTable(data);
      }
   }
   sw.execute();
   /* 
    * Any code run here happens concurrently with doInBackground(). Be careful. 
    * Usually leave empty
    */
}