Le 13/09/2014 19:20, Jacopo Cappellato a écrit :
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? :-)
I did not say that. I was just trying to find a solution and it appeared to me that nothing OOTB was modifying the content of the unmodifiableList
once set, so it was safe. I was not considering OFBiz code as an API. I understand now someone could have used setLocation() in another place. That's
where my reasoning was wrong.
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
Too bad I did not thought about that, it's the right way indeed. I understand
now I was blinded by my own error, when I introduced setLocation()
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 demo instance was not a strong argument indeed. My point was nobody was at risk as long as s/he was not using the -Dportoffset parameter.
setLocation() was then not used, so nothing changed. But I forgot the possible external usage (as an API) of setLocation().
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.
I have fixed that at r1624809 in trunk and r1624814 in R13.07 after
backporting r1624767 there at r1624812
Thanks for your patience
Jacques
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
