Fix for bug #463042

[patrickmhaller]

by avoiding sharing the ListExpressionOperator
instance between threads. This is achieved by
cloning the operator instance in
ArgumentListFunctionExpression.postCopyIn().

Before, getDatabaseStrings() and parallel modifications to
numberOfItems were clashing.

https://bugs.eclipse.org/bugs/show_bug.cgi?id=463042

Change-Id: Ia2e29da74797494394d0323dcc5ded501d72b433
Signed-off-by: Patrick Haller [email protected]

[patrickmhaller]

Ping on the pull request. Please include in an upcoming release.

[patrickmhaller]

@rdicroce and @lukasj Hi - I got stuck with getting this PR included in an Eclipselink release. Could you kindly got a look? Thank you.

[rdicroce]

@patrickmhaller I can't actually do anything here since I'm not a committer. I've just submitted a few patches here and there; in fact I have a PR myself that's been languishing for almost 2 years now (#427). But I noticed the whole codebase was reorganized in master branch to Mavenize the build process, so your patch probably needs to be updated.

[rfelcman]

Sorry, but currently I see there two blockers for this PR

1. Please rebase Your git branch against latest (HEAD) master branch. As @rdicroce mentioned above there was project build transformation from Ant to Maven build system -> sources was moved to another location.
2. I'm missing unit test there. For functional changes/fixes is common practice attach some unit test too. Something similar like test case from https://bugs.eclipse.org/bugs/show_bug.cgi?id=463042 should be fine. Test location should be https://github.com/eclipse-ee4j/eclipselink/tree/master/jpa/eclipselink.jpa.test.jse/src/it/java/org/eclipse/persistence/jpa/test/ + <something> and for test structure check other tests in location e.g. https://github.com/eclipse-ee4j/eclipselink/tree/master/jpa/eclipselink.jpa.test.jse/src/it/java/org/eclipse/persistence/jpa/test

[patrickmhaller]

@rfelcman First, thanks for looking into this.

In regards to unit test, as described in https://bugs.eclipse.org/bugs/show_bug.cgi?id=463042, this is a multi-threading problem.
We used to observe generation of broken SQL in production systems with a certain load. Since this patch was applied, the problems were gone.
Any good idea how to elegantly prove via unit test the correctness of the patch? I'm afraid that multi-threading unit tests are often unreliable and long-running to give enough window of opportunity (and would just prove the EclipseLink code was broken before).
I'd much appreciate if you could kindly read through the issue and review the patch, if possible.

I will try to look into rebasing onto HEAD but it'll me take some time.

[dazey3]

I think a more comprehensive fix for this issue (specifically for master) would be to remove the numberOfItems state from ListExpressionOperator. IMO ExpressionOperators should not track any state and should be shared around. I think this fix is better for patching older releases since we cannot/should not alter existing API there and we should preserve existing behavior, but for master, I would like to see state removed and a new strategy implemented instead.

@patrickmhaller
I opened #1354 and created PRs against 2.6_WAS, 2.7, 3.0, and master. I also added your details to the commits for the service branches as you did isolate where that fix can be implemented. Thanks for the assistance on this!

[patrickmhaller]

Thanks a lot for taking care of this!!