[concurrency-interest] Critique my class ?

Chris Burnley chris.burnley at gmail.com
Thu Sep 22 16:36:02 EDT 2005


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 <tim at peierls.net> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: /pipermail/attachments/20050922/3b885951/attachment.htm


More information about the Concurrency-interest mailing list