Adam Lally wrote:
On 2/9/07, Adam Lally <[EMAIL PROTECTED]> wrote:
This dequeue method still seems fishy to me, even after your fix (not
that your fix broke anything that wasn't already broken). Shouldn't
there by a while (resource == null) instead of just an if?
<snip/>
I forgot to tell you what class that method was in - it is in
BoundedWorkQueue.
The same situation occurs in CPECasPool.getCas().
In both cases the method is marked synchronized, so at first glance it
looks like only one thread can be in there at at time. But since the
method executes this.wait() I think this is not true, since a waiting
thread releases its locks.
Yes, good catch! If there are multiple threads trying to dequeue, they
all wait, and
they all are notified, and wake up, but only the first one gets the
element. The rest
will return null without having waited the full time of their timeout,
perhaps.
Another problem: the notifyAll wakes up all the threads before the
timeout. So,
the next time around in the "while" loop, the timeout value needs to be
adjusted to
compensate for the time already elapsed.
This kind of code occurs in other places, too. Maybe it could be
refactored so
that fixes in one spot would fix all uses.
Another problem I see: the enqueue lets things get added beyond the max
size of the
queue, in several cases (e.g. if enqueuing an EOF token, or if the cpm
is marked not-running).
Once that happens, the enqueue test for queue full fails, and the queue
can grow without bounds. Maybe this never occurs currently, but it's a
risky design.
Maybe it could occur if the CPM is paused? (Not sure about the
semantics of
"not-running")?
-Marshall
