I agree that the fix proposed in the original email is not a good idea.
There are several good books that discuss why DCL should not be used,
and they provide better ways to handle synchronization.
-Adrian
On 1/9/2013 4:15 PM, d...@me.com wrote:
Actually the community is not in agreement that it should be avoided, and in
fact it should be used in many cases.
The reason for this pattern in most of OFBiz is not to guarantee a single
instance of an object but to improve performance, and the synchronized block
with no if block kind of defeats that purpose.
If the code within the synchronized block is run more than once to initialize
the object more than once it's not a big deal and should only happen very
infrequently on init. However, if there is a synchronized block hit many times
this is has a performance impact for the entire time the server is running.
BTW: to really fix this the code pattern below is not adequate... the whole
point of this issue is that an inline synchronized block does not have the same
semantics and thread protection as a synchronized method, which is the proper
fix for this issue.
BTW2: the reason this is a problem in OFBiz is that there is no managed
init/destroy lifecycle. In other words, there is no single object that is
initialized (and later destroyed) for the framework that can initialize (and
destroy) the objects for the various tools like the Entity Engine (delegator)
and Service Engine (dispatcher). Without a managed init/destroy lifecycle
everything is initialized on demand through static methods and with that
approach we're pretty much stuck with this problem.
-David
On Jan 9, 2013, at 5:17 AM, Adrian Crum <adrian.c...@sandglass-software.com>
wrote:
Thank you. DCL has been discussed in the past and the community is in agreement
that it is a pattern to avoid.
If you see DCL code in OFBiz, it is because no one has taken the time to fix it.
-Adrian
On 1/9/2013 1:04 PM, Sumit Pandit wrote:
As per Fortyfy analysis report - "Double-checked locking is an incorrect idiom that
does not achieve the intended effect"
F ollowing code is written to guarantee that only one Fitzer() object is ever
allocated, but does not want to pay the cost of synchronization every time this
code is called. This idiom is known as double-checked locking.
if (fitz == null) {
synchronized (this) {
if (fitz == null) {
fitz = new Fitzer();
}
}
}
return fitz;
Unfortunately, it does not work, and multiple Fitzer() objects can be
allocated. Therefore above code pattern is not recommended by Fortify.
Hence, for above scenario- Following is Recommendation by same: -
synchronized (this) {
if (fitz == null) {
fitz = new Fitzer();
}
}
return fitz;
As it is already known that in terms of performance, synchronized block is
expensive. As OFBiz code contains many references where Double checked locking
existed. And I would go with as is code rather to go with Fortify
recommendation.
Still looking for community to share the opinion. Your comments would be highly
appreciable.
Thanks in advance,
