This discussion is archived
2 Replies Latest reply: Oct 29, 2012 5:37 AM by ryvantage RSS

Is SwingWorker.get() necessary?

ryvantage Newbie
Currently Being Moderated
Hello,

I have been (solo) developing an enterprise database app for the last 3 years and I've been using SwingWorker since day 1 to handle GUI consistency issues. When I began I was fresh out of college with very little experience...

Recently I've begun a sweep of the code (there's a lot of it) to fix my newbie mistakes, and while I was working I began to read on concurrency in Java and I found this article (http://docs.oracle.com/javase/tutorial/uiswing/concurrency/simple.html) and after reading the last paragraph, I am now royally confused... The paragraph says
You may wonder if the code that sets imgs is unnecessarily complicated. Why make doInBackground return an object and use done to retrieve it? Why not just have doInBackground set imgs directly? The problem is that the object imgs refers to is created in the worker thread and used in the event dispatch thread. When objects are shared between threads in this way, you must make sure that changes made in one thread are visible to the other. Using get guarantees this, because using get creates a happens before relationship between the code that creates imgs and the code that uses it. For more on the happens before relationship, refer to Memory Consistency Errors in the Concurrency lesson.
According to this, I have been doing things the wrong way for 3 years, but I have yet (as best of my knowledge) to experience an issue with sharing objects between threads. I have written a small code snippet to illustrate my approach:
    
class SwingWorker1 extends SwingWorker<Void, Void> {

    String[] userList;

    @Override
    protected Void doInBackground() throws Exception {
        userList = databaseObj.getUserList();
    }

    @Override
    protected void done() {
        try {
            get();
        } catch (ExecutionException | InterruptedException e) {
            // Error handling
        }
        printUserList(userList);
    }
}
Yet, according to the article, it should be written like this:
    
class SwingWorker2 extends SwingWorker<String[], Void> {

    String[] userList;

    @Override
    protected String[] doInBackground() throws Exception {
        return databaseObj.getUserList();
    }

    @Override
    protected void done() {
        try {
            printUserList(get());
        } catch (ExecutionException | InterruptedException e) {
            // Error handling
        }
    }
}
My question is this: if done() is executed once doInBackground() is completed (according to the JavaDocs), why then must we use get() to ensure a "happens before" relationship? Is done() itself not sufficient to provide such a situation? Why then have I not experienced failures? I read some articles that led me to suspect it might be because of the particular machines / environments / OS' I've been running my enterprise app on.

I'm interested to hear your thoughts and input. I'm open to criticism of my approach, too. :)
  • 1. Re: Is SwingWorker.get() necessary?
    jtahlborn Expert
    Currently Being Moderated
    your approach and the recommended approach are effectively equivalent. calling "get()" establishes the necessary memory effects, so whether you ultimately get the result from the return value (the recommended approach) or a member variable (your approach) doesn't make a difference.

    looking at the underlying implementation and the various javadocs, i'm not sure why the implementation of the "done()" method is recommended to call "get()", as the guarantees of the done method are that it is run after doInBackground() completes. that alone should establish the necessary "happens before" relationship. but, like i said, your implementation is already perfectly safe as compared to the recommended approach.
  • 2. Re: Is SwingWorker.get() necessary?
    ryvantage Newbie
    Currently Being Moderated
    Ok, great. That's good news. :) A lot of code to be rewritten if my approach is potentially problematic.

    Also, most of what I do doesn't return one type of object. My program has 3 main dashboards with very big SwingWorker methods initializing the dashboards' many objects, not just 1 object here, 1 object there. So to rewrite it, I'd either have to make a separate SwingWorker for every object or I'd have to wrap all of the objects in an array, return the array, and unpack them in done(). Either way seems like a big headache.

    I, too, am wondering though why they insist on using get() inside of done(). Usually that means I'm missing something and the Java devs know something that I don't.