On 5 October 2014 23:39, Xavier Detant <xavier.det...@gmail.com> wrote: > Hello, > > I'm afraid the fix is wrong since, as Bernd said, no JMM property is used. > The JVM is allowed to reorder statments and inlining statments.
I'd forgotten about re-ordering. > The volatile keyword assure you to have the most «fresh» value at the > moment of the reading, but since the System.currentTimeMillis() has no side > effect on the value of lastReturnTime you are not garantee of the > non-inlining of the calculus. Are you sure that the JVM can inline the read of a volatile variable? Also can the JVM re-order statements across a volatile read? > The only way to do it is to make > lastReturnTime field thread-safe using locks. Volatile does make the field thread-safe. It's just a question of whether the JVM can re-order the statements when volatile is used. If the calculation of the elapsed time is synchronised using the same lock as updates to the lastReturnTime field, then of course the problematic window cannot occur. However that is potentially expensive. Is it really necessary if the field is volatile? > I'm new on this project, (and I confess I do not participate a lot to the > apache community) and I just had a look at the class and I think I could > found some time to fix some thread safty issues for this class. > > Greetings > > > 2014-10-05 14:28 GMT+02:00 sebb <seb...@gmail.com>: > >> On 5 October 2014 13:13, Bernd <e...@zusammenkunft.net> wrote: >> > Hmm, >> > >> > I am not sure about this, the local variable fetch does not use any >> > property of the Java Memory Model to make it gurantee to work. >> >> That's not the issue - see my recent update to the JIRA. >> >> > I would >> > simply return 0 if the difference is negative. >> >> Not necessary. >> >> > And of course making the last used value volatile. >> >> +1 >> >> > Greetings >> > Bernd >> > >> > BTW: for what is that idle time used? >> > Am 05.10.2014 13:11 schrieb "Jacopo Cappellato (JIRA)" <j...@apache.org >> >: >> > >> >> >> >> [ >> >> >> https://issues.apache.org/jira/browse/POOL-279?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel >> >> ] >> >> >> >> Jacopo Cappellato updated POOL-279: >> >> ----------------------------------- >> >> Attachment: POOL-279.patch >> >> >> >> New patch with a comment that explain the reason a copy of the field is >> >> taken; thanks to [~s...@apache.org] for the review. >> >> >> >> > Thread concurrency issue in DefaultPooledObject.getIdleTimeMillis() >> >> > ------------------------------------------------------------------- >> >> > >> >> > Key: POOL-279 >> >> > URL: https://issues.apache.org/jira/browse/POOL-279 >> >> > Project: Commons Pool >> >> > Issue Type: Bug >> >> > Affects Versions: 2.2 >> >> > Reporter: Jacopo Cappellato >> >> > Priority: Minor >> >> > Attachments: POOL-279.patch, POOL-279.patch >> >> > >> >> > >> >> > Under unlucky thread concurrency the getIdleTimeMillis() method of >> >> DefaultPooledObject can return a negative value. >> >> > I have attached a Junit test that fails most of the times and a simple >> >> fix, that doesn't use synchronization: with this fix the Junit test >> always >> >> succeed. >> >> >> >> >> >> >> >> -- >> >> This message was sent by Atlassian JIRA >> >> (v6.3.4#6332) >> >> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >> > > > -- > Xavier DETANT > > *"Vi veri veniversum vivus vici*" Johann Georg Faust --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
