As the article and the book (Java Concurrency In Practice) both point out - the
motivation for using DCL (to overcome slow speed) is less of an issue (or a
non-issue) with newer JVMs.
I am not a Java expert. But enough Java experts have stressed the need to
eliminate this anti-pattern that I am willing to take their word for it. I said
that in preparation for repeating what Adam said earlier - there are enough
potential problems with DCL that it should not be used. I agree that there
might be some DCL code in OFBiz that hasn't caused any problems so far, and it
might be tempting to think "if it isn't broke, don't fix it" - but personally,
I prefer to remove the potential problem.
I have seen DCL used in two circumstances in OFBiz: to lazy load a static field
(singleton), and to get elements from a Map.
1. Lazy load a static field
---------------------------
// This is bad, don't do this
public class MyClass {
private static Resource resource;
public static Resource getInstance() {
if (resource == null {
synchronized (MyClass.class) {
if (resource == null) {
resource = new Resource();
}
}
}
return resource;
}
}
The easiest fix in this example is to make getInstance() synchronized:
public synchronized static Resource getInstance() {
if (resource == null {
resource = new Resource();
}
return resource;
}
The lazy load has been preserved, but at a cost of synchronizing every call to
getInstance().
It would be better to eliminate the lazy load altogether:
public class MyClass {
private static Resource resource = new Resource();
public static Resource getInstance() {
return resource;
}
}
2. Get elements from a Map
--------------------------
// This is bad, don't do this
public class MyClass {
private static Map<String, Resource> resourceMap = FastMap.newInstance();
public static Resource getResource(String resourceName) {
Resource resource = resourceMap.get(resourceName);
if (resource == null {
synchronized (MyClass.class) {
resource = resourceMap.get(resourceName);
if (resource == null) {
resource = new Resource();
resourceMap.put(resourceName, resource);
}
}
}
return resource;
}
}
Again, the easiest fix is to make the getResource method synchronized:
public synchronized static Resource getResource(String resourceName) {
Resource resource = resourceMap.get(resourceName);
if (resource == null {
resource = new Resource();
resourceMap.put(resourceName, resource);
}
return resource;
}
This fix comes at a cost of synchronizing every call to getResource. If the
cost of creating a new instance of Resource is low, we can skip synchronization
altogether:
public static Resource getResource(String resourceName) {
Resource resource = resourceMap.get(resourceName);
if (resource == null {
resource = new Resource();
resourceMap.put(resourceName, resource);
resource = resourceMap.get(resourceName);
}
return resource;
}
Using this method, there is a chance that more than one thread will create a
Resource instance, but we are not concerned about that - we just do a second
resourceMap.get(resourceName) method call to make sure we have the Resource
instance that "won."
-Adrian
--- On Sat, 7/3/10, Jacques Le Roux <[email protected]> wrote:
> From: Jacques Le Roux <[email protected]>
> Subject: Re: svn commit: r959875 -
> /ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ModelWidgetAction.java
> To: [email protected]
> Date: Saturday, July 3, 2010, 4:41 AM
> Very good article indeed. What do we
> learn more in the book about DCL?
> If there is nothing more in the book, I suppose we prefer
> to use the static variable solution in OFBiz?
>
> Thanks
>
> Jacques
>
> From: "Adrian Crum" <[email protected]>
> > --- On Fri, 7/2/10, Scott Gray <[email protected]>
> wrote:
> >> The book would have done well to use a more
> complicated
> >> example of why DCL shouldn't be used though. This
> >> article seems to do a better better job of it:
> >> http://www.ibm.com/developerworks/java/library/j-dcl.html
> >
> > That was the article I found a while back - before I
> got the book.
>
>
