Re: [spam] Double-Checked Locking.

[Adrian Crum <ad...@sandglass-software.com>]

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,

Re: Double-Checked Locking.

[Adrian Crum <ad...@sandglass-software.com>]

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, dejc@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 <ad...@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,

Re: [spam] Double-Checked Locking.

[de...@me.com]

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 <ad...@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,
>