Protecting sensitive resources

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

Protecting sensitive resources

Mike Quilleash
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();
        }
    }

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

Re: Protecting sensitive resources

tpeierls
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 <[hidden email]> 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();
        }
    }


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

RE: Protecting sensitive resources

Mike Quilleash
In reply to this post by Mike Quilleash
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: [hidden email] [mailto:[hidden email]] On Behalf Of Tim Peierls
Sent: 09 May 2006 05:24
To: Mike Quilleash
Cc: [hidden email]
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 <[hidden email]> 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();
        }
    }


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

Re: Protecting sensitive resources

tpeierls
On 5/10/06, Mike Quilleash <[hidden email]> wrote:
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.

Nope, it applies to Lock implementations, too. From the javadoc class comment for j.u.c.locks.Lock:

Memory Synchronization

All Lock implementations must enforce the same memory synchronization semantics as provided by the built-in monitor lock:

  • A successful lock operation acts like a successful monitorEnter action
  • A successful unlock operation acts like a successful monitorExit action
Unsuccessful locking and unlocking operations, and reentrant locking/unlocking operations, do not require any memory synchronization effects.


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.

It's not for optimization. It just makes it slightly easier to reason about whether the synchronization policy of your class is correctly observed. Without final, you have to ensure that there is no unguarded access of any kind (write or read) to the openSessions field itself. With final, you only have to worry about avoiding unguarded access to the field's contents.

It's a small thing in this case, but it's a good habit to get into when dealing with larger classes and more complicated synchronization policies: If you can make a field final without contortions, do so. Sometimes it isn't possible, as with the setter injection pattern used by frameworks like Spring, where you should normally use volatile instead.

--tim

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