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

[Joe Walker-3]

Thanks, Tim - not ignoring you just snowed under.

I've taken the Thread.currentThread().interrupt(); on board.

I'm not convinced about the wisdom of skipping onAwakening.run(); in the case of an exception though.
We're in websphere, or something else without async support, we're waiting, holding onto a thread and something wakes us up abnormally. I think we should tell the browser to go away and come back again later, it feels like a much safer thing to do than risk forgetting to ever tell the browser to go away.

Joe.

On Mon, May 5, 2008 at 2:18 PM, Tim Peierls <tim@...> wrote:

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

[tpeierls]

OK, not sure I entirely understand your use case, but if you want ensure that onAwakening is called regardless of interruption status, the behavior should be to restore the interrupt both before and after the call to onAwakening.run(). The after should be in a finally block. This gives the onAwakening code a chance to detect the interrupt and maybe get out quickly, and it ensures that the interrupt is communicated to callers of goToSleep().

It might be better if goToSleep() threw InterruptedException, but that's probably not an easy change to make in a backward-compatible way. If it did, however, you'd restore interrupt before onAwakening and throw IE (in finally block) after it.

--tim

Joe Walker-3 wrote:

Thanks, Tim - not ignoring you just snowed under.

I've taken the Thread.currentThread().interrupt(); on board.

I'm not convinced about the wisdom of skipping onAwakening.run(); in the
case of an exception though.
We're in websphere, or something else without async support, we're waiting,
holding onto a thread and something wakes us up abnormally. I think we
should tell the browser to go away and come back again later, it feels like
a much safer thing to do than risk forgetting to ever tell the browser to go
away.

Joe.


On Mon, May 5, 2008 at 2:18 PM, Tim Peierls <[e-mail]> wrote:

> 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 <[e-mail]> 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
>>
>
>