CODEWITHSUNDEEP
javamultithreadingstaticstatic-methods
Eric
Eric

Reputation: 1973

Java Thread Safety - Lock an entire static class but only for one method

I have a Static helper class implemented that helps cache and retreive some read-only, non-mutable, non-volatile data from the database.

(Stripped) Example:

public class CacheHelper
{
    private static HashMap foos, bars;

    public static Foo getFoo(int fooId) { /* etc etc */ }
    public static Bar getBar(int barId) { /* etc etc */ }

    public static void reloadAllCaches()
    {
        //This is where I need it to lock access to all the other static methods
    }
}

The way I've read it for static classes, If I add the synchronized keyword to the reloadAllCaches() method, this will apply a lock on the entire class while that method executes. Is this correct? (Edit: Yep, not correct. Thanks for the responses. )

Note: I would like to remain agnostic to the thread safety of the getter methods and the objects they return as this data is never mutated and would like it to be returned as fast as possible.

Upvotes: 6

Views: 4451

Answers (5)

Nitram
Nitram

Reputation: 6716

If you add the synchronized keyword to the reloadAllCaches() function all other static functions in the class that got the synchronized keyword can't execute while the reloadAllCaches() function is running.

How ever non-static functions can execute, not matter if they got the synchronized keyword or not. Also all other functions without the synchronized keyword can execute.

After all a function with the synchronized can be looked at like:

public class Bar
{
    public static void foo()
    {
        synchronized (Bar.class)
        {
            // your code
        }
    }
}

A non-static function with the synchronized keyword can be looked at like this:

public class Bar
{
    public void foo()
    {
        synchronized (this)
        {
            // your code
        }
    }
}

So static and non-static functions have a different synchronization context and do not block the execution of each other with the synchronized keyword.

For your case I suggest the usage of a ReentrantReadWriteLock. This class will allow any number of functions to get a read-lock at the same time but only one function to get a Write-Lock. The write lock is only acquired when there is no read-lock in place and no read-lock is acquired as long as a write-lock is in place.

You can make your reload function fetching a write-lock and all your reading function fetching a write-lock. You have to use a static instance of the ReentrantReadWriteLock of cause.

My proposal is to implement it like this:

public class CacheHelper
{
    private static HashMap foos, bars;
    private static java.util.concurrent.locks.ReadWriteLock lock = new java.util.concurrent.locks.ReentrantReadWriteLock();

    public static Foo getFoo(int fooId)
    {
        lock.readLock().lock();
        try {
            /* etc etc */
        } finally {
            lock.readLock().unlock();
        }
    }
    public static Bar getBar(int barId)
    {
        lock.readLock().lock();
        try {
            /* etc etc */
        } finally {
            lock.readLock().unlock();
        }
    }

    public static void reloadAllCaches()
    {
        lock.writeLock().lock();
        try {
            //This is where I need it to lock access to all the other static methods
        } finally {
            lock.writeLock().unlock();
        }
    }
}

Upvotes: 7

Peter Lawrey
Peter Lawrey

Reputation: 533570

If you want the ability to re-populate the collections without locking them, youc an replace them with immutable collections.

private static volatile Map foos, bars;

public static Foo getFoo(int fooId) { return foos.get(fooId); }
public static Bar getBar(int barId) { /* etc etc */ }

public static void reloadAllCaches()
{
    Map newFoo = ...
    // populate newFoo
    foos = newFoo;

    Map newBar = ...
    // populate newBar
    bars = newBar;
}

getFoo will see a complete consistent copy without the need for locks as the Map is always replaced, never modified.

synchronized locks objects not methods, in this case you are locking the CacheHelper.class object

To make the getters fast as possible, you can use a ConcurrentHashMap instead of using synchronized

An example of using synchronized only for updates.

final ConcurrentMap<Key, ExpensiveObject> map =

public ExpensiveObject getOrNull(Key key) { 
     return map.get(key); 
}

public ExpensiveObject getOrCreate(Key key) {
     synchronized(map) {
         ExpensiveObject ret = map.get(key);
         if (ret == null)
              map.put(key, ret = new ExpensiveObject(key));
         return ret;
     }
}

Upvotes: 2

Dave Stubbs
Dave Stubbs

Reputation: 1

In simple terms the lock only prevents other threads from running the same method at the same time, it does not provide any locking resources over any other content in the class, static or otherwise.

Other threads will block access to the method only until the thread that has control exits that method. Access to anything else is still free for all threads.

If you need locking control over the object itself then you'll need to consider providing thread safe accessors or some kind of succession processing for the cache.

By that I mean that if you construct a new cache within this method, and once constructed replace the referenced objects in the cache helper with those new objects, then simply synchronising the reloadAllCaches method will be all you'll need to do.

However, if your intention is to reuse/recycle the existing cache containers then you will have to use locking at the container level to prevent reads while the cache is being destroyed and reconstructed.

If your neeeding to reload multiple cached maps (as per your example) then you may find it necessary to abstract away the cached objects another layer otherwise you might get out of sync access to the caches as you re apply them.

Upvotes: 0

Nathan Hughes
Nathan Hughes

Reputation: 96394

No, this isn't correct. Adding synchronized to only the reloadAllCaches method means that callers of that method must acquire the lock on the class, but threads calling non-synchronized methods can still access the class concurrently. You still need the accessors to be synchronized on the same lock for this to be safe, otherwise the reader threads may not see the latest changes and will get stale data. Alternatively you could use ConcurrentHashMap.

Upvotes: 2

amicngh
amicngh

Reputation: 7899

Rather than applying lock on CacheHelper Class object(CacheHelper.class) in reloadAllCaches() you can apply this lock inside this method on some piece of code because all methods i see are static and if you make them all synchronized then all threads will be blocked if any thread is accessing any method.

Upvotes: 0

Related Questions