Uploaded image for project: 'JGroups'
  1. JGroups
  2. JGRP-1277

Usage of TimeUnit class is inverted, causing incorrect conversions between time units

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Done
    • Icon: Minor Minor
    • 2.12
    • 2.12
    • None
    • Hide

      Taking the example code from the blog post and changing 2000 MILLLISECONDS to 2 SECONDS will show the problem. The 2 SECONDS will instead be converted to 0 which causing an immediate return of tryLock with a false.

      // lock.xml has to have a locking protocol in it
JChannel ch=new JChannel("/home/bela/lock.xml");
LockService lock_service=new LockService(ch);
Lock lock=lock_service.getLock("mylock");
if(lock.tryLock(2, TimeUnit.SECONDS)) {
    try {
        // access the resource protected by "mylock"
    }
    finally {
        lock.unlock();
    }
}
      Show
      Taking the example code from the blog post and changing 2000 MILLLISECONDS to 2 SECONDS will show the problem. The 2 SECONDS will instead be converted to 0 which causing an immediate return of tryLock with a false. // lock.xml has to have a locking protocol in it JChannel ch= new JChannel( "/home/bela/lock.xml" ); LockService lock_service= new LockService(ch); Lock lock=lock_service.getLock( "mylock" ); if (lock.tryLock(2, TimeUnit.SECONDS)) { try { // access the resource protected by "mylock" } finally { lock.unlock(); } }

      I have tried posting something to the blog at http://belaban.blogspot.com/2011/01/new-distributed-locking-service-in.html but it hasn't put up my comment yet, so I decided to create a JIRA as well.

      I have taken the latest 2.12 build of Beta1 which includes the new LockService class. This class is so much of an improvement how the DistributedLockManager was; great work!

      But anyways I found that when you try to acquire a lock with a wait, it is not converting the timeunits correctly unless it is in MILLISECONDS. This cannot be reproduced with the demo class since it always uses MILLISECONDS.

      The code in question is found on the Locking$ClientLock inner class.

              public boolean tryLock(long time, TimeUnit unit) throws InterruptedException {
            return acquireTryLock(unit.convert(time, TimeUnit.MILLISECONDS), true);
        }

      The way this is, the code will convert the time value passed in as MILLISECONDS to the passed in TimeUnit. This simply needs to change to make the TimeUnit.MILLISECONDS the object of which the convert method is being called on and having unit as the second argument.

              public boolean tryLock(long time, TimeUnit unit) throws InterruptedException {
            return acquireTryLock(TimeUnit.MILLISECONDS.convert(time, unit), true);
        }

      This obviously has a simple workaround of always passing in a TimeUnit of MILLISECONDS and having the time value already adjusted.

      And again this new feature looks to be very promising.

      Also when just checking the jgroups code base there are 3 other references to convert, 2 of which are also broken in this fashion.

      HashedTimingWheel.schedule
      TimeScheduler2.schedule

      Both of these references in the jgroup code base are always passed MILLISECONDS, so it would only be seen if client code were to interact with them somehow.

              rhn-engineering-bban Bela Ban
              rpwburns William Burns (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

                Created:
                Updated:
                Resolved: