Can't stop a Runnable once I've passed it to Swing's invokeAndWait within a different thread

Question

I have a class with a fixed thread pool which I use to run a procedure multiple times. Part of the procedure involves creating a Runnable that gets passed to SwingUtilities.invokeAndWait in order to update my Swing GUI.

I want to be able to pre-empt any running procedure when I issue a request to run the procedure again. To this end, I keep the Future from the last submit() call so I can cancel() it. I also keep the last Runnable so I can set a flag on it which tells it to do nothing when run(), since it may have been passed to the AWT event thread and thus I can't stop it by cancelling my Future.

However, no matter what I do, the Runnable gets executed anyway. Here is a stripped-down example:

class Procedure {
    private AtomicReference<Updater> updater;
    private AtomicReference<Future<?>> currProcedure;
    private ExecutorService threadPool;
    private Runnable theProcedure;

    public Procedure() {
        updater = new AtomicReference<Updater>();
        currProcedure = new AtomicReference<Future<?>>();
        threadPool = Executors.newFixedThreadPool(1);
        theProcedure = new Runnable() {
            public void run() {
                doProcedure();
            }
        };
    }
    private void doProcedure() {
        // a bunch of stuff
        updater.set(new Updater());
        try {
            SwingUtilities.invokeAndWait(updater.get());
        } catch (InvocationTargetException | InterruptedException ex) {

        }
        // some more stuff
    }
    public void execute() {
        try {
            synchronized(this) {
                if (null != currProcedure.get()) {
                    currProcedure.get().cancel(true);
                }
                if (null != updater.get()) {
                    updater.get().cancel();
                }
            }
            currProcedure.set(threadPool.submit(theProcedure));
        } catch (RejectedExecutionException ex) {

        }
    }
}

class Updater implements Runnable {
    private AtomicBoolean cancelled;

    public Updater() {
        cancelled = new AtomicBoolean(false);
    }
    public void cancel() {
        cancelled.set(true);
    }
    public void run() {
        if (cancelled.get()) {
            return;
        }
        // do the GUI update
    }
}

It would get used like so:

Procedure p = new Procedure();
p.execute();
p.execute();

In reality, I am calling Procedure.execute() from the AWT event thread, so I think that may have something to do with it; other than that, I have no idea what I am doing wrong or how I can achieve my goal. Any help?

Edit: I have also tried to cancel() my Updater off of the AWT event thread, but no luck anyway:

if (null != currProcedure.get()) {
    currProcedure.get().cancel(true);
}
if (null != updater.get()) {
    threadPool.submit(new Runnable() {
        public void run() {
            updater.get().cancel();
        }
    });
}

Edit 2: My logging statements seem to imply that I successfully interrupt the updater (I get a java.lang.InterruptedException) but the run() executes anyway (and cancelled.get() is still false at the end of it). Why wouldn't cancel(true) stop the execution of the updater?

Answer 1

Your code is broken in several ways. You seem to expect AtomicReference to fix your race conditions magically but all it provides is, as the name suggest, an atomic access to a reference. If you access that object multiple times you have multiple accesses each of them being atomic but together still not being thread safe.

First example:

updater.set(new Updater());
try {
    SwingUtilities.invokeAndWait(updater.get());
} catch (InvocationTargetException | InterruptedException ex) {
}

Both, the set and the following get calls are atomic, but who says that the reference you will receive on get is still the one you have set previously? You might, for example, receive an even newer Updater instance which is scheduled by another thread leading to the situation that one Updater is never sent to the EDT but the other twice.

Second example:

if (null != currProcedure.get()) {
    currProcedure.get().cancel(true);
}
if (null != updater.get()) {
    updater.get().cancel();
}

Same error two times. You are checking the result of AtomicReference.get() against null but being non-null on that invocation, who says that it is still non-null on the next invocation? You have that code in a synchronized block but since the same variables are accessed by other threads, e.g. from within doProcedure() without synchronization, it does not provide any protection. Maybe you never reset the reference to null (which would be another error) so it’s no problem here but it clearly shows a misunderstanding of how to use AtomicReferences.

---

Further, you say you are using it like this:

Procedure p = new Procedure();
p.execute();
p.execute();

So you are invoking execute right one after another and these execute methods try to cancel whatever they find in updater immediately but at this time the background is likely executing your “bunch of stuff” and has not managed to set its updater (as Harald has pointed out). So both execute calls might see null or a very outdated Updater instance and afterwards the background thread sets the first Updater and later on the second Updater, none of them being ever canceled. Note that the interruption of the background thread ends the waiting part of invokeAndWait but does not cancel the execution of the provided runnable.

---

Since you are using AtomicReference you should start really using it. The atomic updates are the key feature to implement the intended logic, e.g.:

Updater newUpdater=new Updater();
Updater old=updater.getAndSet(newUpdater);
if(old!=null) old.cancel();
SwingUtilities.invokeLater(newUpdater);

By reading the old updater and setting the new atomically you ensure that each new Updater is paired with the potential cancellation of an old one. And by keeping the values in local variables instead of reading them multiple times you ensure that there can be no in-between update changing the references. This works even with multiple background threads and does not need additional synchronized blocks.

The call to schedule the Updater has been changed to invokeLater, because in the case of a single threaded executor waiting for the completion of the Updater would imply that the thread never comes to set the new Updater and canceling the old one.

Note that the Updater itself should set the updater reference to null on completion. There is no point in canceling an Updater which has already completed but since the updater reference is a shared reference which might exist far longer than the (intended) life time of the Updater it should be cleared so it can be garbage collected immediately instead of lying around until the next Updater is set.

Answer 2

To me it looks like the problem is in line

// a bunch of stuff

While the bunch of stuff is executed, we have the following situation

1. currProcedure contains the Nth run of theProcedure, while
2. updater only contains the (N-1)st instance of Updater.

Consequently your two cancel calls in the synchronized(true) block do not target a matching pair of items. In fact, the (N-1)st Updater is canceled, but that one is long done. On the other hand the "bunch of stuff" likely does not care about the cancel(true) called on the Future, so once its done, the lines after the "bunch of stuff" are run undeterred and schedule the next Updater.

To solve the situation, the following may help. After the "bunch of stuff", put the following code:

synchronized(this) {
  if (!Thread.currentThread().isInterrupted()) {
    // copy the lines after "a bunch of stuff here to schedule the updater
  }
}

And within execute() you don't need to bother with canceling the Updater. It does not need any cancelation feature.

The two synchronized blocks you now have put a proper ordering into events. Either a fresh execute() makes it into the synchronized block first. Then it can preempt a running doProcedure from scheduling an Updater, or the doProcedure makes it into synchronized first. Well, then the Updater deserves to run, doesn't it? There has to be a point of no return.

I am assuming here that cancel(true) on the Future ends up as an .interrupt() on the underlying thread (not sure currently, look it up). If not, you surely find a way to interrupt the doProcedure thread somehow.