Re: Recent change to ThreadWaitSleeper

by tpeierls :: Rate this Message:

I haven't seen a response to my earlier post, but I had a further thought about it. The current code is (in compressed style):

    public void goToSleep(Runnable onAwakening) {
        try {
            latch.await();
        } catch (InterruptedException ex) {
            Thread.interrupted(); // wrong
        } finally {
            onAwakening.run();
        }
    }

Thread.interrupted() is definitely wrong, but even with Thread.currentThread().interrupt(), this code still runs onAwakening even with an interrupt. Well-behaved code should respond to an interrupt by exiting as quickly as possible after cleaning up. So I think it should be:

    public void goToSleep(Runnable onAwakening) {
        boolean wasInterrupted = false;
        try {
            latch.await();
        } catch (InterruptedException ex) {
            wasInterrupted = true;
            Thread.currentThread().interrupt(); // restore interrupt status
        } finally {
            if (!wasInterrupted) onAwakening.run();
        }
    }

--tim

On Thu, Apr 24, 2008 at 12:42 AM, Tim Peierls <tim@...> wrote:

I just noticed something else that I should have caught before: In ThreadWaitSleeper.java, Thread.interrupted() is wrong; it should be Thread.currentThread().interrupt(). The former clears the interrupt flag and returns whether it was set; the latter sets the flag.

Same thing is true in BlockingMessageListener.receive() in the JMS code. You might also want to consider replacing the use of Object.wait/notify in that code with something else. Likewise in DefaultServerLoadMonitorHarness in the tests.

Apart from that, ThreadWaitSleeper looks a lot cleaner. I hope it isn't meant to be reusable, because CountDownLatches are one-shots: once the latch is open, it can't be closed. A second call to goToSleep() will return immediately. If you need to be able to use it repeatedly, I can suggest an implementation.

Joe Walker wrote:

...indexes are good for keywords, and not for concepts that take a couple of lines to explain.

What would you have looked under? Maybe the next edition can add it.

--tim

« Return to Thread: Recent change to ThreadWaitSleeper