This looks thread-safe to me. However, I'm puzzled by the logic driving the notifications.
In put()
, you wait until the queue is non-full (i.e., at least one slot open), then you add element t
to the end of the queue. Cool, but then you only notify other threads if the queue was empty prior to adding t
. Similarly, in get()
, you wait until the queue is non-empty (i.e., has at least one item), then you fetch element t
from the head of the queue. Again, cool, but then you only notify other threads if the queue was full prior to fetching t
. This will have the effect of requiring the queue to be completely filled before it can be emptied, and conversely for it to be completely emptied before it can be refilled.
Maybe this was your intention, but this is not documented in the behavior, nor is it typical for a blocking queue.
I think what you might want is this instead:
public synchronized void put (T t) throws InterruptedException {
while (isFull())
wait();
q.add(t);
notifyAll();
}
and:
public synchronized T get () throws InterruptedException {
while (isEmpty())
wait();
T t = q.poll();
notifyAll();
return t;
}
This version of put()
will notify waiting threads immediately after a new element is added. This will wake up any waiting get()
calls and immediately fetch the value you just added. This is usually what you want. If any other waiting put()
call wakes up and finds that the queue is full (since you just added an element after possibly having waited for it to be non-full), it will simply cycle around the while
loop and begin waiting again; no harm done.
Similarly, this version of get()
will notify waiting threads immediately after an element is removed. This will wake up any waiting put()
calls immediately and allow them to store a new value. This is also usually what you want. And if any other waiting get()
call wakes up and finds that the queue is empty (since you may have just removed the last element), it will simply cycle around the while
loop and begin waiting again until someone else adds another element.
Other than that, it looks good to me!
p.s. — Since get()
and put()
are synchronized, there's no harm in calling notifyAll()
before you actually remove the item from the queue, because other waiting threads can't actually run again until you exit from get()
(because you're not waiting). So, it can be simplified even further:
public synchronized T get () throws InterruptedException {
while (isEmpty())
wait();
notifyAll();
return q.poll();
}
Queue
, etc.)? Or even just using an existing Blocking Queue? \$\endgroup\$