commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bernd <e...@zusammenkunft.net>
Subject Re: [POOL-279] Thread concurrency issue in DefaultPooledObject.getIdleTimeMillis()
Date Mon, 06 Oct 2014 10:22:30 GMT
Dont worry, currentTimesMillis does not step backward if the timezone
changes automatically, as it is in UTC time.

However it does jump if somebody manually corrects the time (or allows NTP
zu jump the time). Both operations are however a general problem on Java
servers, should be avoided.

If you want to avoid the problem in your code, you can use the monotonic
nanoTime.

Greetings
Bernd
Am 06.10.2014 09:35 schrieb "Jörg Schaible" <joerg.schaible@swisspost.com>:

> Hi,
>
> Jacopo Cappellato wrote:
>
> >
> > On Oct 6, 2014, at 2:31 AM, sebb <sebbaz@gmail.com> wrote:
> >
> >>> 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.
> >
> > I think Xavier's comments are spot on.
> > From what I know, the only JMM guarantee that applies in this context is
> > the  following: "A write to a volatile field happens-before every
> > subsequent read of that same field".
> >
> > On the other hand the "program order rule", as correctly pointed out by
> > Xavier, doesn't guarantee that the method will always return a positive
> > value because the JVM.
> >
> > I am aware of this issue in my fix. I have proposed this fix after I
> > learned that, by design (to avoid synchronization), in this class it was
> > relaxed the thread safe guarantees of a few (non critical) fields.
> >
> > However, the original issue I have identified and exposed in my unit test
> > could only happen under unlucky conditions (I don't know if they are
> > theoretical or real because I don't know enough about how it is used);
> the
> > "incomplete fix" that I have proposed makes the possibility of an error
> > even more remote (I was not able to recreate it with my unit test).
> >
> > In my opinion, if we do not want to implement a fix that involves thread
> > synchronization, we could enhance the fix by including the suggestion
> from
> > Bernd (returning null if the value is negative):
> >
> > Index:
> > src/main/java/org/apache/commons/pool2/impl/DefaultPooledObject.java
> > ===================================================================
> > --- src/main/java/org/apache/commons/pool2/impl/DefaultPooledObject.java
> > (revision 1629471)
> > +++ src/main/java/org/apache/commons/pool2/impl/DefaultPooledObject.java
> > (working copy) @@ -85,7 +85,14 @@
> >
> >      @Override
> >      public long getIdleTimeMillis() {
> > -        return System.currentTimeMillis() - lastReturnTime;
> > +        // Take a copy of lastReturnTime to prevent the risk that
> > +        // a concurrent thread update the value of lastReturnTime
> > +        // between the time that System.currentTimeMillis()
> > +        // is called and before the return value is computed: in
> > +        // this case a negative value could be returned
> > +        long lrTime = lastReturnTime;
> > +        long idleTimeMillis = System.currentTimeMillis() - lrTime;
> > +        return idleTimeMillis > 0? idleTimeMillis: 0;
> >      }
> >
> >      @Override
> >
> >
> > What do you think?
>
> looking at this,I suppose the situation is even worse.
> System.currentTimeMillis will return the elapsed seconds since 1. January
> 1970 calculated based on the system time. Now, if the *system* time
> respects
> daylight saving, System.currentTimeMillis will also - so the value can jump
> back for an hour. System time is also affected by corrections from time
> daemons and might therefore decrease also.
>
> The current recommendation is to use System.nanoTime which calculates the
> elapsed time based on a value calculated at VM startup and that is not
> affected by changes in system time (well, it should not, but some JDK
> versions have errors here). Nevertheless, even in this situation you're not
> safe from negative values in the method above. If you run multi-threaded on
> several cores, the individual cores are not always synchronized by 100%
> (especially if they run at different clock-speeds due power management).
> Depending on the system timer used by the Java VM you are affected.
>
> So be always prepared to get a negative value for such time measuring
> methods.
>
> - Jörg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message