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
