Recent change to ThreadWaitSleeper

[tpeierls]

Revision 1987 has the comment "fixing tomcat reverse ajax bug, which was a nasty synch issue."  I'm looking at the diff and I don't understand what the problem was before, nor how it was fixed.

I do see some problems, though. The boolean wakeUpCalled field should probably be declared volatile, since it is not accessed consistently with a particular lock held -- there is no "x" for which @GuardedBy("x") is true for that field.

The lock objects should be final; it's one less thing to reason about.

More importantly, this looks like something that could be achieved better and more maintainably with higher-level concurrency utilities. A CountDownLatch with intial count of 1, for example: latch.await() followed by runnable.run() for goToSleep(runnable); latch.countDown() for wakeUp().

--tim

[Joe Walker-3]

The problem was that sleepLock.wait() held onto the wakeUpCalledLock for a long time, and wakeup couldn't notify without this lock.

The lack of transient is a typo - it's supposed to be there. The lack of final is an oversight, and the fact that we're not using CountDownLatch is down to me not spending enough time with JCIP. I looked for the answer, but indexes are good for keywords, and not for concepts that take a couple of lines to explain. Now I know the keyword, it's got all I need. :-)

Thanks,

Joe.

On Tue, Apr 22, 2008 at 4:47 AM, Tim Peierls <tim@...> wrote:

Revision 1987 has the comment "fixing tomcat reverse ajax bug, which was a nasty synch issue."  I'm looking at the diff and I don't understand what the problem was before, nor how it was fixed.

I do see some problems, though. The boolean wakeUpCalled field should probably be declared volatile, since it is not accessed consistently with a particular lock held -- there is no "x" for which @GuardedBy("x") is true for that field.

The lock objects should be final; it's one less thing to reason about.

More importantly, this looks like something that could be achieved better and more maintainably with higher-level concurrency utilities. A CountDownLatch with intial count of 1, for example: latch.await() followed by runnable.run() for goToSleep(runnable); latch.countDown() for wakeUp().

--tim

[tpeierls ()]

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.

--tim

Joe Walker-3 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

[tpeierls]

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

[tpeierls]

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