Handling InterruptedException while waiting for an exit signal (bug in Android?)

Question

I've come across the code below, and I'm wondering if it does exactly what I think it does:

synchronized(sObject) {
    mShouldExit = true;   
    sObject.notifyAll()    
    while (!mExited) {
      try {
           sObject.wait();
        } catch (InterruptedException ex) {
           Thread.currentThread().interrupt();
        }
     }
}

About the context: there is another thread that checks for mShouldExit (inside the sObject monitor) and will exit in that case.

This does not look to be a correct pattern to me. If an interrupt happens, it will set the interrupted status again, so when it returns to sObject.wait(), another InterruptedException will come etc. etc. etc. Therefore, it can never go to truly waiting state (sObject.wait()) i.e. it will never release the sObject monitor. This may result in an infinite loop, as the other thread cannot set mExiting to true, because it can never enter sObject's monitor. (So I think that the interrupt() call is an error, it must not be used here.) Am I missing something?

Note that the code snippet is a part of the official Android framework source code.

UPDATE: actually, the situation is worse, because the same pattern is used in Android when your GL rendering starts. The official source code of GLSurfaceView.GLThread.surfaceCreated():

   public void surfaceCreated() {
        synchronized(sGLThreadManager) {
            if (LOG_THREADS) {
                Log.i("GLThread", "surfaceCreated tid=" + getId());
            }
            mHasSurface = true;
            sGLThreadManager.notifyAll();
            while((mWaitingForSurface) && (!mExited)) {
                try {
                    sGLThreadManager.wait();
                } catch (InterruptedException e) {
                    Thread.currentThread().interrupt();
                }
            }
        }
    }

You can reproduce the bug in a similar way: make sure your UI thread has its interrupted status flag yet, then add your GLSurfaceView and start the GL rendering (via setRenderer(...), but on some devices, make sure your GLSurfaceView has Visibility.VISIBLE status, otherwise rendering will not start).

If you follow the above steps, your UI thread will end up in an infinite loop, because the above-quoted code will keep generating an InterruptedException (due to wait()) and therefore the GL thread will never be able to set mWaitingForSurface to false.

According to my tests, it seems that such an infinite loop will also result in an endless sequence of GC_CONCURRENT garbage collection (or, at least, such messages in logcat). Interesting, someone had an unknown poorly-defined issue on stackoverflow earlier which might be related: How to solve GC_concurrent freed?

Isn't it possible that perhaps his UI thread had its interrupted flag set to true, and he was using a GLSurfaceView for the map he mentions? Just an assumption, a possible scenario.

Answer 1

Short version: That code is wrong, and will cause an infinite loop (I still have a doubt, but may depend on JVM implementations). Setting the interrupt status is the right thing to do, but it should then exit the loop, eventually checking that same interruption status using Thread.isInterrupted().

Long version for the casual reader:

The problem is how to stop a thread that is currently executing some work, in response to a "Cancel" button from the user or because of some other application logic.

Initially, Java supported a "stop" method, that preemptively stopped a thread. This method has been demonstrated to be unsafe, cause didn't give the stopped thread any (easy) way to clean up, release resources, avoid exposing partially modified objects and so on.

So, Java evolved to a "cooperative" Thread "interruption" system. This system is quite simple : a Thread is running, someone else calls "interrupt" on it, a flag is set on the Thread, it's Thread responsibility to check if it has been interrupted or not and act accordingly.

So, correct Thread.run (or Runnable.run, of Callable etc..) method implementation should be something like :

public void run() {
  while (!Thread.getCurrentThread().isInterrupted()) {
    // Do your work here
    // Eventually check isInterrupted again before long running computations
  }
  // clean up and return
}

This is fine as long as all the code your Thread is executing is inside your run method, and you never call something that blocks for a long time ... which is often not the case, cause if you spawn a Thread is because you have something long to do.

The simplest method that block is Thread.sleep(millis), it's actually the only thing it does : it blocks the thread for the given amount of time.

Now, if the interrupt arrives while your thread is inside Thread.sleep(600000000), without any other suport, it would take a lot for it to arrive to the point where it checks isInterrupted.

There are even situations where your thread would never exit. For example, your thread is computing something and sending results to a BlockingQueue with a limited size, you call queue.put(myresult), it will block until the consumer free some space in the queue, if in the mean time the consumer has been interrupted (or died or whatever), that space will never arrive, the method will not return, the check on .isInterrupted will never be performed, your thread is stuck.

To avoid this situation, all (most) methods that interrupt the thread (should) throw InterruptedException. That exception simply tells you "I was waiting for this and that, but in the meanwhile the thread as been interrupted, you should do cleanup and exit as soon as possible".

As with all exceptions, unless you know what to do, you should re-throw it and hope that someone above you in the call stack knows.

InterruptedExceptions are even worse, since when they are thrown the "interrupted status" is cleared. This means that simply catching and ignoring them will result in a thread that usually does not stop :

public void run() {
  while (!Thread.getCurrentThread().isInterrupted()) {
    try {
      Thread.sleep(1000);
    } catch (InterruptedException e) {
      // Nothing here
    }
  }
}

In this example, if the interrupt arrives during the sleep() method (which is 99.9999999999% of the time), it will throw InterruptedException, clear the interrupt flag, then the loop will continue since the interrupt flag is false, and the thread will not stop.

That's why if you implement your "while" correctly, using .isInterrupted, and you really need to catch InterruptedException, and you don't have anything special (like cleanup, return etc..) to do with it, least that you can do is set the interrupt flag again.

The problem in the code you posted is that the "while" relies solely on mExited to decide when to stop, and not ALSO on isInterrupted.

while (!mExited && !Thread.getCurrentThread().isInterrupted()) {

Or it could exit when interrupted :

} catch (InterruptedException e) {
  Thread.currentThread().interrupt();
  return; // supposing there is no cleanup or other stuff to be done
}

Setting the isInterrupted flag back to true is also important if you don't control the Thread. For example, if you are in a runnable which is being executed in a thread pool of some kind, or inside any method anywhere you don't own and control the thread (a simple case : a servlet), you don't know if the interruption is for "you" (in the servlet case, the client closed the connection and the container is trying to stop you to free the thread for other requests) or if it's targeted at the thread (or system) as a whole (the container is shutting down, stopping everything).

In that situation (which is 99% of the code), if you cannot rethrow the InterruptedException (which is, unfortunately, checked), the only way to propagate up the stack to the thread pool that the thread has been interrupted, is setting the flag back to true before returning.

That way, it will propagate up the stack, eventually generating more InterruptedException's, up to the thread owner (be it the jvm itself, of an Executor, or any other thread pool) that can react properly (reuse the thread, let it die, System.exit(1) ...)

Most of this is covered in chapter 7 of Java Concurrency in Practice, a very good book that I recommend to anyone interested in computer programming in general, not just Java, cause the problems and the solutions are similar in many other environments, and explanations are very well written.

Why Sun decided to make InterruptedException checked, when most documentation suggests to rethrow it mercilessly, and why they decided to clear the interrupted flag when throwing that exception, when the proper thing to do is setting it to true again most of the time, remains open for debate.

However, if .wait releases the lock BEFORE checking for the interrupt flag, it open a small door from another thread to modify the mExited boolean. Unfortunately the wait() method is native, so source of that specific JVM should be inspected. This does not change the fact that the code you posted is coded poorly.