Critique my class ?

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Critique my class ?

Chris Burnley
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).


class Switch {

    private Lock lock = new ReentrantLock();

    private Condition condition = lock.newCondition();

    private boolean waiters;
    private Exception failure;
   
    /**
     * 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;
    }

    /**
     * Unlock the switch and notify all those that have been waiting.
     */
    public void unlockAndSignalAll() {
        waiters = false;
        condition.signalAll();
        lock.unlock();
    }
   
    /**
     * Indicate to waiters that the operation has failed.
     *
     */
    public void signalFailure(Exception cause){
        this.failure = cause;
        unlockAndSignalAll();
    }

    /**
     * Used by waiters on the switch.
     * @throws InterruptedException
     */
    public void await() throws InterruptedException,  WaitFailureException{
        lock.lock();
        try {
            waiters = true;
            condition.await();
            if(failure != null)
                throw new WaitFailureException(failure);
        } finally {
            lock.unlock();
        }
    }
}

The clients call this method after they've written to the channel:


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



and I have a scheduled task that does the actual file sync:

    class FileSyncer implements Runnable {

        public void run() {
            while (swtch.lockIfWaiters()) {
                try {
                    fileChannel.force(false);
                    swtch.unlockAndSignalAll();
                } catch (Exception e){
                    // error logging omitted
                    swtch.signalFalure(e);
                }
            }
        }
    }

regards,

Chris Burnley

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

RE: Critique my class ?

Peter Veentjer - Anchor Men
 
Would it not be an idea the store all the requests for every thread in the queue and periodically flush that queue? Now clients don`t have to block untill they can write information to the channel.You can use a bounded BlockingQueue to prevent that there are to many requests and you can use a ScheduledExecutor to schedule a runnable for the flusing task.

 

Van: [hidden email] [mailto:[hidden email]] Namens Chris Burnley
Verzonden: donderdag 22 september 2005 12:09
Aan: [hidden email]
Onderwerp: [concurrency-interest] Critique my class ?

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).


class Switch {

    private Lock lock = new ReentrantLock();

    private Condition condition = lock.newCondition();

    private boolean waiters;
    private Exception failure;
   
    /**
     * 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;
    }

    /**
     * Unlock the switch and notify all those that have been waiting.
     */
    public void unlockAndSignalAll() {
        waiters = false;
        condition.signalAll();
        lock.unlock();
    }
   
    /**
     * Indicate to waiters that the operation has failed.
     *
     */
    public void signalFailure(Exception cause){
        this.failure = cause;
        unlockAndSignalAll();
    }

    /**
     * Used by waiters on the switch.
     * @throws InterruptedException
     */
    public void await() throws InterruptedException,  WaitFailureException{
        lock.lock();
        try {
            waiters = true;
            condition.await();
            if(failure != null)
                throw new WaitFailureException(failure);
        } finally {
            lock.unlock();
        }
    }
}

The clients call this method after they've written to the channel:


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



and I have a scheduled task that does the actual file sync:

    class FileSyncer implements Runnable {

        public void run() {
            while (swtch.lockIfWaiters()) {
                try {
                    fileChannel.force(false);
                    swtch.unlockAndSignalAll();
                } catch (Exception e){
                    // error logging omitted
                    swtch.signalFalure(e);
                }
            }
        }
    }

regards,

Chris Burnley

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

RE: Critique my class ?

Peter Veentjer - Anchor Men
In reply to this post by Chris Burnley
And you could use a Future of you realy need to wait for the result of the execution.


Van: [hidden email] [mailto:[hidden email]] Namens Peter Veentjer - Anchor Men
Verzonden: donderdag 22 september 2005 13:17
Aan: Chris Burnley; [hidden email]
Onderwerp: RE: [concurrency-interest] Critique my class ?

 
Would it not be an idea the store all the requests for every thread in the queue and periodically flush that queue? Now clients don`t have to block untill they can write information to the channel.You can use a bounded BlockingQueue to prevent that there are to many requests and you can use a ScheduledExecutor to schedule a runnable for the flusing task.

 

Van: [hidden email] [mailto:[hidden email]] Namens Chris Burnley
Verzonden: donderdag 22 september 2005 12:09
Aan: [hidden email]
Onderwerp: [concurrency-interest] Critique my class ?

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).


class Switch {

    private Lock lock = new ReentrantLock();

    private Condition condition = lock.newCondition();

    private boolean waiters;
    private Exception failure;
   
    /**
     * 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;
    }

    /**
     * Unlock the switch and notify all those that have been waiting.
     */
    public void unlockAndSignalAll() {
        waiters = false;
        condition.signalAll();
        lock.unlock();
    }
   
    /**
     * Indicate to waiters that the operation has failed.
     *
     */
    public void signalFailure(Exception cause){
        this.failure = cause;
        unlockAndSignalAll();
    }

    /**
     * Used by waiters on the switch.
     * @throws InterruptedException
     */
    public void await() throws InterruptedException,  WaitFailureException{
        lock.lock();
        try {
            waiters = true;
            condition.await();
            if(failure != null)
                throw new WaitFailureException(failure);
        } finally {
            lock.unlock();
        }
    }
}

The clients call this method after they've written to the channel:


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



and I have a scheduled task that does the actual file sync:

    class FileSyncer implements Runnable {

        public void run() {
            while (swtch.lockIfWaiters()) {
                try {
                    fileChannel.force(false);
                    swtch.unlockAndSignalAll();
                } catch (Exception e){
                    // error logging omitted
                    swtch.signalFalure(e);
                }
            }
        }
    }

regards,

Chris Burnley

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Critique my class ?

Chris Burnley
The whole point is that it is a XA database so each request must be gauranteed that the sync has occurred and therfore will block. however, new waiters that began waiting after the sync was started must not be notified with the group of requests that began waiting before the sync was started.

On 9/22/05, Peter Veentjer - Anchor Men <[hidden email]> wrote:
And you could use a Future of you realy need to wait for the result of the execution.


Van: [hidden email] [mailto:[hidden email]] Namens Peter Veentjer - Anchor Men
Verzonden: donderdag 22 september 2005 13:17
Aan: Chris Burnley; [hidden email]
Onderwerp: RE: [concurrency-interest] Critique my class ?

 
Would it not be an idea the store all the requests for every thread in the queue and periodically flush that queue? Now clients don`t have to block untill they can write information to the channel.You can use a bounded BlockingQueue to prevent that there are to many requests and you can use a ScheduledExecutor to schedule a runnable for the flusing task.

 

Van: [hidden email] [mailto:[hidden email]] Namens Chris Burnley
Verzonden: donderdag 22 september 2005 12:09
Aan: [hidden email]
Onderwerp: [concurrency-interest] Critique my class ?

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).


class Switch {

    private Lock lock = new ReentrantLock();

    private Condition condition = lock.newCondition();

    private boolean waiters;
    private Exception failure;
   
    /**
     * 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;
    }

    /**
     * Unlock the switch and notify all those that have been waiting.
     */
    public void unlockAndSignalAll() {
        waiters = false;
        condition.signalAll();
        lock.unlock();
    }
   
    /**
     * Indicate to waiters that the operation has failed.
     *
     */
    public void signalFailure(Exception cause){
        this.failure = cause;
        unlockAndSignalAll();
    }

    /**
     * Used by waiters on the switch.
     * @throws InterruptedException
     */
    public void await() throws InterruptedException,  WaitFailureException{
        lock.lock();
        try {
            waiters = true;
            condition.await();
            if(failure != null)
                throw new WaitFailureException(failure);
        } finally {
            lock.unlock();
        }
    }
}

The clients call this method after they've written to the channel:


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



and I have a scheduled task that does the actual file sync:

    class FileSyncer implements Runnable {

        public void run() {
            while (swtch.lockIfWaiters()) {
                try {
                    fileChannel.force(false);
                    swtch.unlockAndSignalAll();
                } catch (Exception e){
                    // error logging omitted
                    swtch.signalFalure(e);
                }
            }
        }
    }

regards,

Chris Burnley

_______________________________________________
Concurrency-interest mailing list
[hidden email]
<a onclick="return top.js.OpenExtLink(window,event,this)" href="http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest" target="_blank">http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

RE: Critique my class ?

Peter Veentjer - Anchor Men
In reply to this post by Chris Burnley
I have made an example of what I mean:
 
interface ChannelCommand{
 void run(Channel channel);
}
 

class ChannelCommandWriter{
 
 private ScheduledExecutorService _executor = new ThreadPoolExecutorService();
 private BLockingQueue<FutureTask> _futureQueue = new LinkedBlockingQueue<FutureTask>();
 private Channel _channel = ...;
 
 public ChanneldCommandWriter(){
  _executor.scheduleAtFixedDelay(new FlushRunnable(),0,50,TimeUnit.MILISECONDS);
 }
 
 public Future execute(final ChannelCommand command){
  Runnable r = new Runnable(){
   void run(){
    command.run(_channel);
   }   
  }; 
 
  FutureTask<Object> ftask = new FutureTask<Object>(r, null);
  _futureQueue.add(ftask);
  return ftask;         
 }
 
 private class FlushRunnable implements Runnable{
  public void run(){
   List<Future> futureList = new LinkedList<Future>();
   _futureQueue.draintTo(futureList);
   
   for(Future future: futureList){
    future.run();
   }
  }
 }
}
 
 
 
And now you can use it asynchronous:
 
ChannelCommandWriter writer = new ChannelCommandWriter();
writer.execute(someChannelCommand);
 
or synchronous:
 
ChannelCommandWriter writer = new ChannelCommandWriter();
Future future = writer.execute(someChannelCommand);
future.get();//wait untill the execution completed.


Van: [hidden email] [mailto:[hidden email]] Namens Chris Burnley
Verzonden: donderdag 22 september 2005 14:12
Aan: [hidden email]
Onderwerp: Re: [concurrency-interest] Critique my class ?

The whole point is that it is a XA database so each request must be gauranteed that the sync has occurred and therfore will block. however, new waiters that began waiting after the sync was started must not be notified with the group of requests that began waiting before the sync was started.

On 9/22/05, Peter Veentjer - Anchor Men <[hidden email]> wrote:
And you could use a Future of you realy need to wait for the result of the execution.


Van: [hidden email] [mailto:[hidden email]] Namens Peter Veentjer - Anchor Men
Verzonden: donderdag 22 september 2005 13:17
Aan: Chris Burnley; [hidden email]
Onderwerp: RE: [concurrency-interest] Critique my class ?

 
Would it not be an idea the store all the requests for every thread in the queue and periodically flush that queue? Now clients don`t have to block untill they can write information to the channel.You can use a bounded BlockingQueue to prevent that there are to many requests and you can use a ScheduledExecutor to schedule a runnable for the flusing task.

 

Van: [hidden email] [mailto:[hidden email]] Namens Chris Burnley
Verzonden: donderdag 22 september 2005 12:09
Aan: [hidden email]
Onderwerp: [concurrency-interest] Critique my class ?

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).


class Switch {

    private Lock lock = new ReentrantLock();

    private Condition condition = lock.newCondition();

    private boolean waiters;
    private Exception failure;
   
    /**
     * 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;
    }

    /**
     * Unlock the switch and notify all those that have been waiting.
     */
    public void unlockAndSignalAll() {
        waiters = false;
        condition.signalAll();
        lock.unlock();
    }
   
    /**
     * Indicate to waiters that the operation has failed.
     *
     */
    public void signalFailure(Exception cause){
        this.failure = cause;
        unlockAndSignalAll();
    }

    /**
     * Used by waiters on the switch.
     * @throws InterruptedException
     */
    public void await() throws InterruptedException,  WaitFailureException{
        lock.lock();
        try {
            waiters = true;
            condition.await();
            if(failure != null)
                throw new WaitFailureException(failure);
        } finally {
            lock.unlock();
        }
    }
}

The clients call this method after they've written to the channel:


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



and I have a scheduled task that does the actual file sync:

    class FileSyncer implements Runnable {

        public void run() {
            while (swtch.lockIfWaiters()) {
                try {
                    fileChannel.force(false);
                    swtch.unlockAndSignalAll();
                } catch (Exception e){
                    // error logging omitted
                    swtch.signalFalure(e);
                }
            }
        }
    }

regards,

Chris Burnley

_______________________________________________
Concurrency-interest mailing list
[hidden email]
<A onclick="return top.js.OpenExtLink(window,event,this)" href="http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest" target=_blank>http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Critique my class ?

tpeierls
In reply to this post by Chris Burnley
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

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Critique my class ?

Chris Burnley
Thanks Tim for your really well thought out critism. I'll take your suggestions on board. This is the sort of stuff I was after to try and make it solid.

I've actually got a class called ObjectFile which contains the FileChannel, this is where the force() method lives ( although I didn't make that clear because I didn't think it mattered).

This FileSyncer is an inner class (mainly becuase I didn't want to expose run() to clients) of ObjectFile started up by the ExecutorService.

The reason I've got the loop in FileSyncer in there is that I seemed to get higher throughput by reducing the schedule rate to about 100ms, and increasing the effictive rate scheduling rate by using the loop , this also has the effect of reducing the mean latency of clients. I'll have to experiment on what effects this has on other resources, otherwise I might go back to a if statement.

Thanks again,

Chris

On 9/22/05, Tim Peierls <[hidden email]> wrote:
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



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Critique my class ?

Gregg Wonderly-2
In reply to this post by tpeierls


Tim Peierls wrote:

> Chris Burnley wrote:
>      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());

The ugly pattern that I always use is to create the wrapper exception with the message from the thrown exception that I
caught, and then call initCause() with the caught exception.

        throw (SyncFailedException)new SyncFailedException(e.toString()).initCause(e);

This gets a little long to type.  A standard annotation would be nice for this pattern.

Gregg Wonderly
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest