Le 13/09/2014 06:24, Jacopo Cappellato a écrit :
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).
I checked this aspect (OOTB code only of course). ServiceLocation.getLocation() is only used in AbstractEngine.createLocationMap() where it depends on
ServiceConfigUtil.getServiceEngine().getServiceLocations().
Since ServiceEngine.serviceLocations is a Collections.unmodifiableList created in the synchronized block there are no read threats, that's the
purpose of the unmodifiableList.
Immutability is the best solution to guarantee threadSafe, but it's not the
only one.
With this commit you have introduced a regression, and also the inline
documentation (see the @ThreadSafe tag) of the ServiceLocation class is now
misleading.
Ha indeed I missed the @ThreadSafe tag :/
Unfortunately with my solution it should be removed, and even a comment should
be added.
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.
I don't want to revert. I have created
https://issues.apache.org/jira/browse/OFBIZ-5770 and attached a patch.
Would you have a better solution which would keep ServiceLocation threadSafe?
For other readers, again: this is currently only a possible problem if you use the -Dportoffset parameter. I never crossed issue personnaly and the
official stable demo is running with it for few months w/o noticed issues.
Jacques
Jacopo
On Sep 13, 2014, at 12:15 AM, Jacques Le Roux <[email protected]>
wrote:
Le 12/09/2014 19:33, Jacopo Cappellato a écrit :
On Sep 12, 2014, at 7:24 PM, Jacques Le Roux<[email protected]>
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
