On Sep 13, 2014, at 11:14 AM, Jacques Le Roux <[email protected]> wrote:
> > 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. Are you teaching me how to write thread safe code? :-) Jacques, the list in unmodifiable but the objects in it are modifiable (because of your commit): this breaks the pattern. > >> 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. Your patch is a wrong approach: if you synchronize the writes but you do not synchronize the reads the code is still not thread safe; I already told you this but you don't read carefully what I am writing you and you keep replying with incorrect statements. > Would you have a better solution which would keep ServiceLocation threadSafe? yes, of course I do; I have committed it with rev. 1624767 > > 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. The demo instances with the low traffic they experience aren't a good test bed for testing how the system deal with concurrent threads. The portOffset code you have committed in OFBiz has hardcoded port numbers and works only if you run it with the default configuration; it should be reverted completely. Jacopo > > 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 >> >> >
