[concurrency-interest] Protecting sensitive resources

Mike Quilleash mike.quilleash at azuresolutions.com
Wed May 10 05:29:20 EDT 2006


Thanks for your reply.
 
I agree using wait()/notify() is far simpler and I've changed it to be
similar to your solution below.  I guess the only minor downside is that
with two wait conditions a notifyAll() will wake all the waiting threads
even if their condition hasn't been met but this class isn't heavily
travelled enough to worry about that.
 
A couple of questions, your comment about the boolean not needing to be
volatile is that only when using the standard java synchronized, or does
it also apply to the Lock method too?  I was having difficulty seeing
how the lock implementation could figure out what variables need to be
synched up so maybe for Lock you have to use volatile.
 
What advantages does making the openSessions field final have?  Is it
for the JVM to optimise and/or protect the field itself from getting
modified, just the Set contents.
 
Thanks again for you time.
 
Mike.


________________________________

From: tpeierls at gmail.com [mailto:tpeierls at gmail.com] On Behalf Of Tim
Peierls
Sent: 09 May 2006 05:24
To: Mike Quilleash
Cc: concurrency-interest at cs.oswego.edu
Subject: Re: [concurrency-interest] Protecting sensitive resources


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/20060510/f7d104e4/attachment.html


More information about the Concurrency-interest mailing list