Singleton Instance variables concurrency
Below I have code from a class that is used for caching data from a database. The class is implemented as a singleton.
My question deals with the method getCacheObj(). Does getCacheObj() return a copy of the reference to cacheObj or does it return the actual cacheObj variable itself?
Specifically I am concerned about a concurrency issue when the methods refreshCache() and getCacheObj() are called simultaneously. What would happen in the following situation:
Thread 1 calls findActiveByWebsiteArea() which calls getCacheObj(). getCacheObj() returns the cacheObj for processing down the line in a jsp or other java object. Prior to thread 1 finishing the processing with cacheObj, thread 2 calls refreshCache() which resets cacheObj. Is it possible for thread 2 to clash with thread 1?
public class NewsPeer extends AncestorCachePeer {
// object that will house the cached data
protected static NewsPeer cacheObj = null;
// attributes for the instantiated cache object
private Hashtable newsByWebsiteArea = null;
private NewsPeer() {
// private constructor to enforce a singleton
}
public static void refreshCache() throws Exception {
// reset the cache object so a fresh retrieve will be performed
// the next time the cache is accessed
synchronized (cacheObj) {
cacheObj = null;
}
}
private static NewsPeer getCacheObj() throws Exception {
synchronized (cacheObj) {
if (cacheObj == null) {
cacheObj = new NewsPeer();
cacheObj.retrieveCache();
}
return cacheObj;
}
}
public static List findActiveByWebsiteArea(String websiteareaId) throws Exception{
// get item from cache obj
return (List) getCacheObj().newsByWebsiteArea.get(websiteareaId);
}
private void retrieveCache() throws Exception {
// code to populate newsByWebsiteArea on cacheObj
}
}
it returns a copy of the reference
private static NewsPeer getCacheObj() throws Exception {
synchronized (cacheObj) {
if (cacheObj == null) {
cacheObj = new NewsPeer();
cacheObj.retrieveCache();
}
Just an FYI that this code is bad. The reason being broken JMM. You can do a search on google for "double checked locking" and you would find tons of stuff on this. As an appetizer try this :-
http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double.html
and yes please use code tags while posting code
Thanks for the reminder about wrapping code with code tags.
I read the article on JMM. When I look at my code, it appears to be correct since I use the synchronize key word everywhere the cacheObj is accessed. Both in getCacheObj() and refreshCache(). Can you enlighten me on what I might be missing.
Thanks
Well, I don't really see a DCL problem here, but a non working synchronized statement, as the cacheObj might be null on the first call and will throw a NullPointerException.
Other than that, as test and assignment is done within the synchronized block, there is no DCL but low performance synchronization.
> Well, I don't really see a DCL problem here, but a
> non working synchronized statement, as the cacheObj
> might be null on the first call and will throw a
> NullPointerException.
A standard singleton would have just synchronized on the class, wouldn't it? Like this:private synchronized static NewsPeer getCacheObj() throws Exception {
if (cacheObj == null) {
cacheObj = new NewsPeer();
cacheObj.retrieveCache();
}
return cacheObj;
}
Believe it or not, I had my code like that. However I read somewhere that when you have a static synchronized method, that the entire class is locked. I understand this to mean that other threads wouldn't be able to call other static methods on the same class until the lock is released. I wanted my code to be efficient so I put the synchronized lock on the cacheObj instead of the entire class.
Is this the proper way to handle my concerns?
> Believe it or not, I had my code like that. However
> I read somewhere that when you have a static
> synchronized method, that the entire class is locked.
That's true. Except for the word "entire", which is inflammatory and misleading. The class is locked, yes. Other things related to the class, like members of the class and objects referred to by static variables, are not locked. Just the class itself.
> I understand this to mean that other threads
> wouldn't be able to call other static methods on the
> same class until the lock is released.
Only synchronized static methods. If you only have this one synchronized static method, then only calls to it will be synchronized.
> I wanted my code to be efficient so I put the synchronized lock
> on the cacheObj instead of the entire class.
> Is this the proper way to handle my concerns?
No. As was already (sort of) pointed out earlier, you have to have an object to synchronize on. You can't synchronize on that NewsPeer object until it actually exists. Your code is trying to synchronize on a variable.
Is the following code kosher? Specifically I'm looking at the logic inside of the private constructor for NewsPeer where I set NewsPeer.cacheObj to null if an exception occurs. Also I'm wondering about the instantiation for the static variable cacheObj on the NewsPeer class.
Since I have multiple static methods, I was thinking that I should use synchronized code inside the static method(s) that modify newsByWebsiteArea verses making the entire method static. I was thinking this would prevent an unnecessary lock on the class when staticMethod2() and staticMethod3() were called. Is this legitimate?
public class NewsPeer extends AncestorCachePeer {
// object that will house the cached data
protected static NewsPeer cacheObj = new NewsPeer();
// attributes for the instantiated cache object
private Hashtable newsByWebsiteArea = null;
private NewsPeer() {
// private constructor to enforce a singleton
try {
this.retrieveCache();
} catch (Exception e) {
e.printStackTrace();
NewsPeer.cacheObj = null;
}
}
public static void refreshCache() throws Exception {
synchronized (cacheObj) {
cacheObj.retrieveCache();
}
}
public static List findActiveByWebsiteArea(String websiteareaId) throws Exception{
// get item from cache obj
synchronized (cacheObj) {
return (List) cacheObj.newsByWebsiteArea.get(websiteareaId);
}
}
protected void retrieveCache() throws Exception {
// retrieve items that will be cached
newsByWebsiteArea = new Hashtable();
...
// more code to manipulate newsByWebsiteArea
...
}
public static void staticMethod2() throws Exception {
// method that does not manipulate newsByWebsiteArea
...
}
public static void staticMethod2() throws Exception {
// method that does not manipulate newsByWebsiteArea
...
}
}
You only ever synchronize on cacheObj, in your proposal. So doing that gains you exactly nothing over synchronizing on the class, and it's flawed because you can't synchronize on null. But then you've got a lot of other potential NullPointerExceptions waiting to happen there.
I doubt that cacheObj will ever be set to null, as the assignment within the constructor will happen before the assignment of the instance you are creating at that right moment. You can still synchronize on the class or a third object for providing the locking/synchronized access. Or use some of the locking mechanisms, if you are using Java5.
Can someone review this code for me and tell me if it's going to accomplish what I'm looking for. I re-hashed the use of the singleton design pattern and decided that I might not need it (I was modeling after some other example code and perhaps went down the wrong road). My questions are as follows:
1) Do I really need to have a singleton to house the cached data, or is it kosher to have the static variable newsByWebsiteArea on the class itself?
2) Will my code require synchronized methods for findActiveByWebsiteArea() and findActiveByLocation()? I don't want to have issues inside these methods while the refreshCache() method is running.
3) If I have other non-synchronized static methods in this class, will the thread calling the non-synchronized methods have to wait until the synchronized methods are finished executing? If so, would it be a better idea to go back to synchronizing blocks of code instead of methods to ensure maximum efficiency?
Thank you for your help.
public class NewsPeer extends AncestorCachePeer {
// attributes for the instantiated cache object
private static Hashtable newsByWebsiteArea = null;
private static Hashtable newsByLocation = null;
static {
try {
refreshCache();
} catch (Exception e) {
e.printStackTrace();
}
}
public synchronized static List findActiveByWebsiteArea(String websiteareaId) throws Exception{
// get item from cache obj
return (List) newsByWebsiteArea.get(websiteareaId);
}
public synchronized static List findActiveByLocation(String locationId) throws Exception{
// get item from cache obj
return (List) newsByLocation.get(locationId);
}
public synchronized static void refreshCache() throws Exception {
// retrieve items that will be cached
newsByWebsiteArea = new Hashtable();
// additional code to populate newsByWebsiteArea
newsByLocation = new Hashtable();
// additional code to populate newsByLocation
}
> 1) Do I really need to have a singleton to house the
> cached data, or is it kosher to have the static
> variable newsByWebsiteArea on the class
> itself?
That IS a singleton. You have exactly one instance of something. That's what a singleton is. It's true that you don't have any instances of NewsPeer, but you do have exactly one instance of the things you're interested in.
> 2) Will my code require synchronized methods for
> findActiveByWebsiteArea() and
> findActiveByLocation()? I don't want to have
> issues inside these methods while the
> refreshCache() method is running.
Yes.
> 3) If I have other non-synchronized static
> methods in this class, will the thread calling the
> non-synchronized methods have to wait until the
> synchronized methods are finished executing?
No. Only synchronized methods or blocks respect the locks held by other synchronized methods or blocks.
> If so, would it be a better idea to go back to synchronizing
> blocks of code instead of methods to ensure maximum
> efficiency?
Your earlier examples synchronized on the entire method's code. I don't understand why you would think that would work differently than just synchronizing the method itself. The lock is held for exactly the same code.
Now if the critical block was only part of the method, you might want to synchronize on that. But in your case it isn't.
However: you actually have two independent objects there. So you could do something like this: public class NewsPeer extends AncestorCachePeer {
// attributes for the instantiated cache object
private static Hashtable newsByWebsiteArea = new Hashtable();
private static Hashtable newsByLocation = new Hashtable();
static {
try {
refreshCache();
} catch (Exception e) {
e.printStackTrace();
}
}
public static List findActiveByWebsiteArea(String websiteareaId) throws Exception{
synchronized(newsByWebsiteArea) {
// get item from cache obj
return (List) newsByWebsiteArea.get(websiteareaId);
}
}
public static List findActiveByLocation(String locationId) throws Exception{
synchronized(newsByLocation) {
// get item from cache obj
return (List) newsByLocation.get(locationId);
}
}
public static void refreshCache() throws Exception {
// retrieve items that will be cached
synchronized(newsByWebsiteArea) {
newsByWebsiteArea.clear();
// additional code to populate newsByWebsiteArea
}
synchronized(newsByLocation) {
newsByLocation.clear();
// additional code to populate newsByLocation
}
}
That way the two "find" methods don't block each other. (Note that I changed your code slightly so you don't create new objects in the refresh() method. That's because the revised code synchronizes on the objects in question, so creating new objects would lead to synchronization failures.
Or as stefan.schulz suggests, you could use Java 5 synchronization tools to improve this. I don't know much about them but I'm pretty sure there's something that lets readers share the data without blocking and only blocks when writers are active. That's probably worth looking into.
Yes. If you're in using Hashtable, in Java5 you might want to replace it by ConcurrentHashMap, which does some locking on relevant operations (and actually not on the whole Map) so you won't get problems with concurrent usages. In the example given by DrClap, you would not need to synchronize on the Hashtables. Especially, if those objects solely are used for caching.
Btw. the Hashtable implementation in Java5 is fully synchronized ...
Message was edited by:
stefan.schulz
