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 <[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
