[concurrency-interest] Critique my class ?

Tim Peierls tim at peierls.net
Thu Sep 22 15:44:27 EDT 2005


Chris Burnley wrote:
> I have a use case where I've got several client threads that write to a 
> FileChannel but I must guarantee that the file gets synced for each 
> client. A file sync could take up to 40ms depending on the size of the 
> file. To get around this I want to buffer up the clients so I only have 
> to do the sync once every, say 50ms, thus drastically improving the 
> throughput at the cost of a little latency.
> 
> Question : is there something already available that would help me do 
> this sort of thing ? I thought maybe a cyclicbarrier but somehow it 
> didn't fit ( I'm not concerned with the number of clients waiting but 
> rather the time elapsed since last sync)
> 
> Otherwise, can anybody see anything wrong with the following 
> implementation ? (I've called it Switch for want of a better term).


Looks pretty reasonable. I made some specific comments below.

A nice way to package this would be to extend FileChannel to delegate to a "real" FileChannel, 
with force(boolean) overridden to work like Switch.force() in your code. Then the 
ScheduledExecutorService that runs FileSyncer periodically could be encapsulated inside the 
FileChannel implementation. Clients would just call the usual force method (you'd have to find a 
way to communicate the need for a file metadata refresh to the Switch) and catch a custom 
SyncFailedException that extends IOException.

If you do this, then there's no need to provide Switch on its own.



>     private Lock lock = new ReentrantLock();
>     private Condition condition = lock.newCondition();

Make these final. Two fewer things to worry about.


>     private boolean waiters;
>     private Exception failure;

Any particular reason not to keep a count of waiters? Might be useful for debugging, and might 
allow a small optimization if the count is 1.

   private int waiters;

And won't you want to catch Throwable in await(), not just Exception? If your call to 
FileChannel.force(false) throws OutOfMemoryError, aren't the awaiters going to want to know that?

   private Throwable failure;

You should document that waiters and failure (the mutable state of Switch) are guarded by lock. 
Simplifies reasoning for others who read the code.


>     /**
>      * Lock, but only if there are waiters.
>      * @return if the lock was acquired.
>      */
>     public boolean lockIfWaiters() {
>         lock.lock();
>         failure = null;
>         if (!waiters) {
>             lock.unlock();
>             return false;
>         }
>         return true;
>     }

Feels safer to have try-finally in anything that does a lock-unlock, even though I suppose it 
doesn't make any difference in this case. But this way you can see clearly that waiters and 
failure are accessed only with the lock held.

   boolean acquire = false;
   lock.lock();
   try {
       failure = null;
       acquire = waiters > 0;
       return acquire;
   } finally {
       if (!acquire) lock.unlock();
   }



>     public void unlockAndSignalAll() {

Document the precondition that lock is held by this thread.

>         waiters = false;
>         condition.signalAll();
>         lock.unlock();
>     }

       waiters = 0;


>     public void signalFailure(Exception cause){

Document the precondition that lock is held by this thread.

   public void signalFailure(Throwable cause){


>     public void await() throws InterruptedException,  WaitFailureException{
>         lock.lock();
>         try {
>             waiters = true;

   ++waiters;

>             condition.await();

Always call await() in a while loop! You could have a spurious wakeup; you need a condition 
predicate to decide whether the return from await is for real. In this case, the condition for 
continuing is "no waiters".

               while (waiters > 0) condition.await();

>             if(failure != null)
>                 throw new WaitFailureException(failure);
>         } finally {
>             lock.unlock();
>         }
>     }
> }


>     protected void force() throws SyncFailedException {
>         try {
>             swtch.await();
>         } catch (InterruptedException e) {
>             throw new SyncFailedException(e.toString());

If the *client* is interrupted you have two choices: propagate the InterruptedException or restore 
the interrupted status. In this case, since a client that calls force knows that it could block 
for a while, propagating is better:

      protected void force() throws InterruptedException, SyncFailedException {

and remove the InterruptedException catch clause.


>         } catch (WaitFailureException e) {
>             throw new SyncFailedException(e.toString());       

If you don't have to flatten the failure into string, don't. How about:

   throw new SyncFailedException(e.getCause());

That's assuming SyncFailedException is a class you control.


>     class FileSyncer implements Runnable {
>         public void run() {
>             while (swtch.lockIfWaiters()) {
>                 try {
>                     fileChannel.force(false);
>                     swtch.unlockAndSignalAll();
>                 } catch (Exception e){
>                     // error logging omitted
>                     swtch.signalFailure(e);
>                 }
>             }
>         }
>     }

Do you necessarily want to keep looping on signalFailure? There's a chance that blocked waiters 
will be able to do something with the knowledge that the sync failed, like cancel any further 
attempts. Maybe lockIfWaiters could test the failure field and throw immediately if not null. 
You'd have to provide a clear failure method to allow things to proceed again.

--tim



More information about the Concurrency-interest mailing list