[concurrency-interest] Protecting sensitive resources

Tim Peierls tim at peierls.net
Tue May 9 00:23:36 EDT 2006


It looks OK to me. I find the explicit Lock/Condition stuff difficult to
read. Are you sure you need uninterruptible waits here? If not, and if you
are willing to stick IE in the throws clause of initialise and openSession,
plain wait/notifyAll is easier to read and reason about.

// set of currently open sessions
@GuardedBy("this") private final Set<UsageSession> openSessions =
    new HashSet<UsageSession>();
@GuardedBy("this") private boolean refreshing = false;
// CONDITION-PREDICATE: noOpenSessions (openSessions.isEmpty())
// CONDITION-PREDICATE: notRefreshing (!refreshing)

// BLOCKS-UNTIL: noOpenSessions
synchronized void initialise() throws InterruptedException {
    refreshing = true;
    while (!openSessions.isEmpty()) wait();
    refreshing = false;
    notifyAll(); // notRefreshing

    // TODO: do initialisation here
}

// open a usage session for use
// BLOCKS-UNTIL: notRefreshing
public synchronized UsageSession openSession() throws InterruptedException {
    while (refreshing) wait();

    UsageSession result = new UsageSession(this);
    openSessions.add(usageSession);
    return result;
}

// call back made from the usage session when it is closed
synchronized void releaseSession(UsageSession usageSession) {
    if (!openSessions.remove(usageSession))
        throw new IllegalArgumentException( "Attempted to release unknown
usage session" );
    if (openSessions.isEmpty())
        notifyAll(); // noOpenSessions
}

The boolean refreshing field doesn't need to be volatile since all access to
it is done while holding the lock.

Also, why set openSessions to a new empty hash map when it is already empty?
By sticking with the same set instance, you can make openSessions a final
field.

--tim

On 5/8/06, Mike Quilleash <mike.quilleash at azuresolutions.com> wrote:
>
>  Hi there,
>
> I have a class that is responsible for delegating resources to other
> classes.
>
> Snapshot of the code below...
>
> initalise() handles refreshing the internal state (other internal
> variables which are not shown).
> openSession() should create a new session and return it (unless currently
> refreshing).
> releaseSession() should return a session to the class and mark it as no
> longer used.
>
>
> The requirements for how this class should work is...
>
> The class should keep track of current in-use session objects
> (UsageSession) which must be threadsafe.
> UsageSessions must be returned to the class when they are no longer used.
> The class may be reinitialised at any time in which case:
>     It should not refresh until all Sessions have been returned.
>     It should not allow any new sessions to be delegated out until
> reinitialise is complete.
>
>
> I'd appreciate any feedback or problems anyone sees with the
> implementation below, I've stared at it for a while now and run through the
> different possible scenarios of thread states etc but can't see any
> problems.
>
> Appreciate it.
>
> Mike Quilleash.
>
>
>
>
>
>     // set of currently open sessions
>     private Set< UsageSession > openSessions;
>
>     // locks and conditions protecting the open sessions list
>     private Lock openSessionsLock = new ReentrantLock();
>     private Condition openSessionsEmptyCondition =
> openSessionsLock.newCondition();
>     private Condition openSessionsNotRefreshingCondition =
> openSessionsLock.newCondition();
>     private volatile boolean refreshing = false;
>
>     void initialise()
>     {
>         openSessionsLock.lock();
>         try
>         {
>             // set refreshing flag
>             refreshing = true;
>
>             // wait for the open sessions list to be empty
>             while( openSessions.size() > 0 )
>             {
>                 openSessionsEmptyCondition.awaitUninterruptibly();
>             }
>
>             // clear out the open sessions - will already be empty
>             openSessions = new HashSet< UsageSession >();
>
>             // TODO: do initialisation here
>
>             // clear refreshing flag
>             refreshing = false;
>
>             // signal all threads waiting on this condition
>             openSessionsNotRefreshingCondition.signalAll();
>         }
>         finally
>         {
>             openSessionsLock.unlock();
>         }
>     }
>
>     // open a usage session for use
>     public UsageSession openSession()
>     {
>         // lock on the open sessions list
>         openSessionsLock.lock();
>         try
>         {
>             // if refreshing then wait for the refreshing condition
>             while ( refreshing )
>             {
>                 // wait on the not refreshing condition
>                 openSessionsNotRefreshingCondition.awaitUninterruptibly();
>             }
>
>             UsageSession usageSession = new UsageSession( this );
>
>             openSessions.add( usageSession );
>
>             return usageSession;
>         }
>         finally
>         {
>             openSessionsLock.unlock();
>         }
>     }
>
>     // call back made from the usage session when it is closed
>     void releaseSession( UsageSession usageSession )
>     {
>         openSessionsLock.lock();
>         try
>         {
>             if ( !openSessions.remove( usageSession ) )
>                 throw new IllegalArgumentException( "Attempted to release
> unknown usage session" );
>
>             // signal that the open sessions set is empty incase something
> is waiting
>             if ( openSessions.isEmpty() )
>                 openSessionsEmptyCondition.signalAll();
>         }
>         finally
>         {
>             openSessionsLock.unlock();
>         }
>     }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: /pipermail/attachments/20060509/99f43bfc/attachment-0001.html


More information about the Concurrency-interest mailing list