I believe it's clean now
Jacques
Le 13/09/2014 20:32, Adrian Crum a écrit :
I recall when this feature was proposed, there was some opposition to it. I had suggested using patches instead of making code modifications, but
that idea was rejected.
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 9/13/2014 6:20 PM, Jacopo Cappellato wrote:
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
