Com.g3d.app.SimpleApplication not thread-safe

Contribution Depot
1

The method start in SimpleApplication isn't thread-safe. A simple test with substance look and feel (that includes a thread-safe checking system). The creation of the SettingsDialog should be delegated to the EDT Thread. A possible workaround would be to use an asynchronous approach. I post a (rater ugly) example. The start method becomes:


    @Override
    public void start(){
        // show settings dialog
        if (showSettings){
            final Object MONITOR = new Object();
            final AtomicBoolean release = new AtomicBoolean(false);
            final AtomicBoolean accept = new AtomicBoolean(false);
            java.awt.EventQueue.invokeLater(new Runnable() {
                public void run() {
                    URL iconUrl = SimpleApplication.class.getResource("Monkey.png");
                    SettingsDialog dialog = new SettingsDialog(settings, iconUrl);
                    final Runnable unlocker = new Runnable() { public void run() {
                        synchronized(MONITOR) {
                            release.set(true);
                            MONITOR.notifyAll();
                        }
                    }};
                    Runnable onAccept = new Runnable() { public void run() {
                        accept.set(true);
                        unlocker.run();
                    }};
                    Runnable onCancel = new Runnable() { public void run() {
                        accept.set(false);
                        unlocker.run();
                    }};
                    dialog.showDialog(onAccept, onCancel);
                }
            });
            if(!java.awt.EventQueue.isDispatchThread()) {
                synchronized(MONITOR) {
                    while(!release.get()) {
                        try {
                            MONITOR.wait();
                        } catch(InterruptedException ex) {
                            ex.printStackTrace();
                            return;
                        }
                    }
                    if(accept.get()) {
                        super.start();
                    }
                }
            } else {
                super.start();
            }
        } else {
            super.start();
        }
    }[code]

In SettingsDialog one can introduce a couple of fields and a different show method:

[code]    private Runnable onAccept, onCancel;
    public void showDialog(Runnable onAccept, Runnable onCancel) {
        this.onAccept = onAccept;
        this.onCancel = onCancel;
        showDialog();
    }[/code]
   
And the action listeners for the ok and cancel buttons call the runnables:

[code]        ok.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                if (verifyAndSaveCurrentSelection()){
                    setUserSelection(APPROVE_SELECTION);
                    dispose();
                    if(onAccept != null) {
                        onAccept.run();
                    }
                }
            }
        });

        cancel.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                setUserSelection(CANCEL_SELECTION);
                dispose();
                if(onCancel != null) {
                    onCancel.run();
                }
            }
        });[/code]
       
It is just a rough sketch of one possible solution.[/code]

2

So you're saying that JFrames should be created in the EDT thread? I am sure it's possible to achieve this by putting a few fields here and there, I would prefer not to clutter SimpleApplication with this stuff, preferably SettingsDialog should take care of that part maybe through a static method.

3

The " A simple test with substance look and feel (that includes a thread-safe checking system)." was "A simple test with substance … confirmed it".



Not just the JFrame but any AWt/Swing component. There is a dead lock risk (due to UI delegates if i remember well) - long time ago Sun posted an example where such situation occurred in a five lines program involving a JTable.



A static method in SettingsDialog would be great. Well, anything that doesn't clutter the public api is great for me. It isn't less convoluted. in fact there is a bug in the solution i posted, due to the unsynchronized nature of HashMap (and AppSettings).



If in SettingsDialog-createUI we add:



setModalityType(Dialog.ModalityType.APPLICATION_MODAL);



i think that a static "show" method could look like this:


    public static boolean showDialog(AppSettings sourceSettings, final URL iconURL) {
        final AtomicBoolean answer = new AtomicBoolean();
        final AtomicBoolean release = new AtomicBoolean();
        final Object lock = new Object();
        final AppSettings settings;
        synchronized(lock) {
            settings = new AppSettings(false);
            settings.copyFrom(sourceSettings);
        }
        java.awt.EventQueue.invokeLater(new Runnable() {
            public void run() {
                SettingsDialog dialog;
                synchronized(lock) {
                    dialog = new SettingsDialog(settings, iconURL);
                }
                dialog.showDialog();
                synchronized(lock) {
                    AppSettings temp = new AppSettings(false);
                    temp.copyFrom(dialog.source);
                    settings.copyFrom(temp);
                    release.set(true);
                    answer.set(dialog.selection == SettingsDialog.APPROVE_SELECTION);
                    lock.notifyAll();
                }
            }
        });
        if(!java.awt.EventQueue.isDispatchThread()) {
            synchronized(lock) {
                while(!release.get()) {
                    try {
                        lock.wait();
                    } catch(InterruptedException ex) {
                        return false;
                    }
                }
            }
        }
        synchronized(lock) {
            sourceSettings.copyFrom(settings);
        }
        return answer.get();
    }


   
And the start method in SimpleApplication become:

    @Override
    public void start(){
        // show settings dialog
        if(showSettings) {
            URL iconURL = SimpleApplication.class.getResource("Monkey.png");
            boolean accepted = SettingsDialog.showDialog(settings, iconURL);
            if(!accepted) {
                return;
            }
        }

        super.start();
    }


   
   
In fact the "show" method must deal with three issues.

1. creating SettingsDialog in the EDT, due to the AWT/Swing thread policy.
2. blocking the caller of show if it is not the EDT
3. ensuring that sourceSettings values are visible to the edt and the changes made by the edt to the its settings are visible to the caller of show.

I can't think of a cleaner solution that doesn't touch AppSettings too.
4

afaik the EDT-limitation is only valid for windows that are actually visible. So every modification of the window before setVisible(true) could be done on any thread. Maybe that can ease the situation a bit?

5

The edt rule holds for non realized windows too (if they are meant to be realized). It's just a matter of threads. Sooner or later the EDT will have that window - and its fields - in his hands. If you create the window outside the edt then you will have to deal with unsynchronized access to shared values.

6
pgi said:

The edt rule holds for non realized windows too (if they are meant to be realized). It's just a matter of threads. Sooner or later the EDT will have that window - and its fields - in his hands. If you create the window outside the edt then you will have to deal with unsynchronized access to shared values.


He is correct. The book "Filthy Rich Clients" (written by two long-time Swing developers) specifically warns about this.