Re: [j-devel] Upcoming abcl-0.0.10 release

[Peter G. <pe...@ar...>]

On Wed, 21 Feb 2007 at 22:02:09 +0200, Alex Mizrahi wrote:
> (message (Hello 'Peter)
> (you :wrote  :on '(Wed, 21 Feb 2007 09:58:51 -0800))
> (
>
>  PG> In the case where the box is empty and you have to wait, the first
>  PG> version releases the lock after wait() returns, then re-acquires the
>  PG> lock and calls box.isEmpty() again before calling removeFirst().
>
>  PG> The second version calls removeFirst() immediately after exiting the
>  PG> while loop, which should be OK since you still hold the lock when
>  PG> wait() returns.
>
>  PG> Am I missing something?
>
> you are right, but please also change method send() to do notifty() instead 
> of notifyAll();
> that will allow to use multiple cosumer threads (pool of threads that 
> perform tasks, for example).
> notify() wakes exactly one thread that will return from read().
>
> i'm not an expert in multithreaded Java programming, so i didn't know about 
> details of wait() and notify().
> with notifyAll() it was possible that multiple threads do wait(). then 
> notifyAll will wake them all, they will one-by-one re-aquire lock and do 
> box.removeFirst(), so second thread will fail with NoSuchElementException. 
> with my version of patch it did re-check that box is not empty.
>
> but using notify() to handle multiple-threads case certainly would be more 
> efficient.

I've committed this part:

  private LispObject read()
  {
      synchronized(this)
        {
          while (box.isEmpty())
            {
              try
                {
                  wait();
                }
              catch(InterruptedException e)
                {
                  throw new RuntimeException(e);
                }
            }
          return (LispObject) box.removeFirst();
        }
  }

I haven't changed send(); I think notifyAll() is still correct.

When any thread returns from wait(), it is still holding the lock. The
flow of control reaches the bottom of the while loop, but then it goes
back and tests box.isEmpty() again at the top of the while loop before
falling through to box.removeFirst(), just as in your original patch.

OK?

-Peter