ofbiz-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacopo Cappellato <jacopo.cappell...@gmail.com>
Subject Re: svn commit: r1533839 - in /ofbiz/trunk: ./ framework/base/src/org/ofbiz/base/container/ framework/catalina/src/org/ofbiz/catalina/container/ framework/common/servicedef/ framework/service/config/ framework/service/src/org/ofbiz/service/config/model/ fr...
Date Sat, 13 Sep 2014 04:24:30 GMT
If you are suggesting to synchronize access to the location field during writing but not during
reading then this is wrong approach (this is the very first basic concept you should know
when you touch a thread-safe class).
With this commit you have introduced a regression, and also the inline documentation (see
the @ThreadSafe tag) of the ServiceLocation class is now misleading.

And please no, don't do further work on this (unless you would like to revert your commit),
it is better for OFBiz and the community if I take care of fixing this stuff.

Jacopo

On Sep 13, 2014, at 12:15 AM, Jacques Le Roux <jacques.le.roux@les7arts.com> wrote:

> 
> Le 12/09/2014 19:33, Jacopo Cappellato a écrit :
>> On Sep 12, 2014, at 7:24 PM, Jacques Le Roux<jacques.le.roux@les7arts.com>
 wrote:
>> 
>>> this.serviceLocations = Collections.unmodifiableList(serviceLocations);
>> Exactly: this pattern can only work under the assumption of the immutability of the
serviceLocation objects; since you broke it the classes are no more thread safe.
>> 
>> Jacopo
>> 
> 
> Immutability is not required, as long as we can guarantee that all threads will always
have the same services engines locations values.
> BTW from a practical perspective this only had an impact when the -Dportoffset parameter
was used. Else the services engines locations were actually not changed, once read from their
"service-location"
> 
> I suggest to synchronize the content of the else code block which begins at
>    List<ServiceLocation> serviceLocations = new ArrayList<ServiceLocation>(serviceLocationElementList.size());
> and ends at
>    this.serviceLocations = Collections.unmodifiableList(serviceLocations);
> 
> Since ServiceLocation.setLocation() is only used there (in OOTB code at least) we would
be sure that all threads will always have the right values, even if the -Dportoffset parameter
is used.
> The performance impact should not be huge. I guess initializing service engines is not
often done, mostly (if not only, I could not verify completly) at start.
> 
> Jacques


Mime
View raw message