i sent it to the list on the commit thread that it was not needed and leading to close twice the pool which is an issue so just wrapped it in a finally (other parts of the commit were introducing regressions as explained in the thread).
Since we start to lock the pool the race condition shouldnt occur there but there was a little chance under start[abrupt ctrl+x] case to not close properly the pool so added a finally. Also this method is not intended to be used in a concurrent environment (actually issue would be higher level if you underploy concurrently the same app which is more than unlikely. It is an undeploy method bound to one bean so no supported concurrency there - except in tests?). Anyway since it is there since > 5 years and not causing any security issue it is not a cancel reason I think. Romain Manni-Bucau @rmannibucau <https://twitter.com/rmannibucau> | Blog <https://blog-rmannibucau.rhcloud.com> | Old Blog <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> | LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory <https://javaeefactory-rmannibucau.rhcloud.com> 2017-03-08 11:10 GMT+01:00 Andy Gumbrecht <[email protected]>: > -1 > > https://github.com/rmannibucau/tomee/commit/55d6d5ec89da68a6794487bc75f95b > 55cad7b483 > > Not sure why the public boolean close(final long timeout, final TimeUnit > unit) throws InterruptedException { method doesn't look like this one in > 1.7.x?: > https://github.com/apache/tomee/blob/tomee-1.7.x/ > container/openejb-core/src/main/java/org/apache/openejb/util/Pool.java > - Was pretty sure I checked it in on master too. Looks like an older commit > and not the last one I have? My fault, as I guess I didn't push to master. > > The stop() method also needs to be called before draining (in the close > method), else there is a potential race. > > The atomics must be set to null on stop(), so getAndSet is better as it is > a single atomic action rather than two (get, compare = 4 locks in total). > > Basically the tomee-1.7.x Pool stop() and close() methods are correct and > tested (also on site), and can be forwarded to master. I won't be able to > fix master till I get home tonight, so if someone could compare the > tomee-1.7.x methods and update master that would be cool. > > Andy. > > > On 8 March 2017 at 08:32, Romain Manni-Bucau <[email protected]> > wrote: > > > Hi guys, > > > > as discussed on the list here is the vote for 7.0.3. It is mainly > > dependencies upgrades and a few fixes. > > > > Staging repo: > > https://repository.apache.org/content/repositories/orgapachetomee-1104/ > > Source zip: > > https://repository.apache.org/content/repositories/ > > orgapachetomee-1104/org/apache/tomee/tomee-project/7. > > 0.3/tomee-project-7.0.3-source-release.zip > > Dist area: https://dist.apache.org/repos/dist/dev/tomee/7.0.3/ > > Changelog: > > https://issues.apache.org/jira/browse/TOMEE-2018?jql= > > project%20%3D%20TOMEE%20AND%20fixVersion%20%3D%207.0.3% > > 20AND%20(resolution%20%3D%20Resolved%20OR%20resolution%20%3D%20Fixed) > > Green buildbot: > > https://ci.apache.org/builders/tomee-trunk-ubuntu-jvm8/builds/603 > > Branch: https://github.com/rmannibucau/tomee/tree/release/7.0.3 > > > > Please vote: > > - +1: it rocks, release it > > - +-0: why do you bother me? > > - -1 don't release it cause ${blocker} > > > > As usual vote will be open for 3 days or until we get 3 +1 bindings and > no > > -1. Anyone is welcomed to vote! > > > > Romain Manni-Bucau > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > > <https://blog-rmannibucau.rhcloud.com> | Old Blog > > <http://rmannibucau.wordpress.com> | Github <https://github.com/ > > rmannibucau> | > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory > > <https://javaeefactory-rmannibucau.rhcloud.com> > > > > > > -- > Andy Gumbrecht > https://twitter.com/AndyGeeDe > http://www.tomitribe.com >
