How do I guarantee the complete construction of an object in the presence of threads

Question

My reading of JLS 12.5 makes me think the assertion in the code sample should never trigger—but it does in my multithreaded code. (The JLS doesn't specify threading in the section). But whether or not my reading is correct is beside the point. I want to get this to always be true.

public class MainWindow extends JFrame {
  private final JLabel label;

  public MainWindow() {
    label = new JLabel();
    pack();
    setVisible(true);
  }

  public JLabel getLabel() {
    Assert.assertNotNull(label);
    return label;
  }
}

The obvious answer is to wrap the innards of the constructor in a synchronized block and to mark the getter synchronized as well. Is there a better way?

FWIW, the other thread is getting a reference to the window with this code inside a junit test:

private MainWindow findMainWindow() {
    for (Frame frame : Frame.getFrames()) {
        if (frame instanceof MainWindow) {
            return (MainWindow)frame;
        }
    }
    return null;
}

(BTW, I'm running JDK6 on a Mac)

Update:

I even tried synchronizing it and it still doesn't work. Here's the code:

public class MainWindow extends JFrame {
  private final JLabel label;
  public MainWindow() {
    synchronized(this) {
        label = new JLabel();
    }
  }

  public synchronized JLabel getLabel() {
    Assert.assertNotNull(label);
    return label;
  }
}

Update 2:

Here's the change that fixed it:

private MainWindow findMainWindow() throws Exception {
    final AtomicReference<MainWindow> window = new AtomicReference<MainWindow>();
    SwingUtilities.invokeAndWait(new Runnable() {
        public void run() {
            for (Frame frame : Frame.getFrames()) {
                if (frame instanceof MainWindow) {
                    window.set((MainWindow) frame);
                    return;
                }
            }
        }
    });
    return window.get();
}

I don't know innards of Swing, but I suggest problem would be solved by moving `pack();setVisible(true)` from ctor to separate method -- seems like `this` escaped to other thread currently. — Victor Sorokin, Jul 08 '11 at 11:42
is `Frame.getFrames()` even safe to call outside of the EDT? — jtahlborn, Jul 08 '11 at 11:51
I think Victor Sorokin has the explanation of the problem, and that jtahlborn has the solution: don't use Swing controls outside of the Event Dispatch Thread. Read http://download.oracle.com/javase/6/docs/api/javax/swing/package-summary.html#threading — JB Nizet, Jul 08 '11 at 11:58
though I'm still curious why synchronizing didn't do it. I'm guessing it's that race condition between object construction and the ctor's synchronized block. Though, it is very strange to be hitting that race condition _every_ time. — Michael Deardeuff, Jul 08 '11 at 12:13
so, um, jtahlborn or JB Nizet, would you like to make your comment into an answer and I'll accept it. — Michael Deardeuff, Jul 08 '11 at 21:23

score 1 · Answer 1 · answered Jul 08 '11 at 23:57

There is no way to guarantee "label" has a value in "getLabel". Well, actually I guess there are several, but that's the wrong way to approach the problem.

The problem is that somewhere you have an instance field declaration like:

private MainWindow  mainWindow;

and somewhere (and it better run on the EventQueue, or you will have other troubles) a statement like:

mainWindow = new MainWindow();

Once this statement starts to execute, "mainWindow" has a non-null reference to the space where the data for a MainWindow object will be placed. Due to the bad luck that plagues all multi-threaded programs, at this point in another thread the following code gets executed:

MainWindow  mw = mainWindow;
if (mw != null)  label = mw.getLabel();

Your assertion is triggered. Then your original thread very thoughtfully locks while the constructor code runs.

The Solution: Make "mainWindow" a volatile. That will force the compiler to be sure the MainWindow object is completed before mainWindow gets its value. Also, while it should not be necessary, I like to keep references to instance fields to an absolute minimum and to keep them as simple as possible, so I'd have the code looking like this:

private volatile MainWindow  mainWindow;

MainWindow  mw = new MainWindow();
mainWindow = mw;

Do this in all similar cases. Whenever you assign a value to an instance or class field accessed from more than one thread, make sure what you are assigning is ready to be seen by all other threads.

(If you can get the call to "getLabel" onto the EventQueue also, you can forget about all this and live in single-threaded bliss.)

score 1 · Accepted Answer · answered Jul 14 '11 at 21:11

(Note: if one of the originators of this answer actually answers it, I will un-accept this answer and accept theirs instead.)

The window frame is being constructed off the EDT. This against Swing policy where all access—including creation—should happen on the EDT.

I now create the window like this (using my EventQueue utility class):

MainWindow ui = EventQueue.invokeAndGet(new Callable() {
    public MainWindow call() {
        return new MainWindow(...);
    }
});

score 0 · Answer 3 · answered Jul 08 '11 at 11:46

0

One way might be to make the contructor private and provide a factory method to return a new instance.

Something like:

public static final Object lockObject = new Object();
public static final MainWindow createWindowInstance() {
  synchronized(lockObject) {
    MainWindow win = new MainWindow();
    return win;
  }
}

This is off the top of my head so the code is more psuedo code.

answered Jul 08 '11 at 11:46

Mark Miller

143
7

The problem is actually that the window is being accessed before the constructor returns. The root cause: the window isn't being created on the EDT. Moving all access to the EDT (including construction) fixes the problem. – Michael Deardeuff Jul 14 '11 at 20:51

How do I guarantee the complete construction of an object in the presence of threads

3 Answers3