About JDK-6772009

[David Holmes]

Hi,

You seem to be looking at an older version of the test code.

As I said in my previous mail I think the problem is that an interrupted 
thread can follow this path:

    if (!interrupted) {
       if (print)
          System.out.printf("completed after %d millis%n", 
                           NANOSECONDS.toMillis(System.nanoTime() - 
startTime));
       lock.lock();
       try {
          ++completed;
       }
       finally {
          lock.unlock();
       }
     }

because interrupted is only set when InterruptedException is caught:

while (!done || Thread.currentThread().isInterrupted()) {
     try {
         lock.lockInterruptibly();
     }
     catch (InterruptedException ie) {
         interrupted = true;

but we can also exit the loop due to interruption, without recording the 
fact we were interrupted. In which case the completed count will be too 
high.

Cheers,
David

On 5/01/2017 2:39 PM, chen zero wrote:
> Hi David,
> Thank you for reminding.
> I post the code here.
>
>
> import java.util.Arrays;
> import java.util.Collections;
> import java.util.Random;
> import java.util.concurrent.BrokenBarrierException;
> import java.util.concurrent.CyclicBarrier;
> import java.util.concurrent.locks.ReentrantLock;
>
> // https://bugs.openjdk.java.net/browse/JDK-6772009
>
> public final class CancelledLockLoops {
>     static final Random rng = new Random();
>     static boolean print = false;
>     //static final int ITERS = 1000000;
>     static final int ITERS = 10000000;
>
>     static final long TIMEOUT = 100;
>
>     public static void main(String[] args) throws Exception {
>         int maxThreads = 5;
>         if (args.length > 0) {
>             maxThreads = Integer.parseInt(args[0]);
>         }
>
>         print = true;
>
>         for (int i = 2; i <= maxThreads; i += (i+1) >>> 1) {
>             System.out.print("Threads: " + i);
>
>             try {
>                 new ReentrantLockLoop(i).test();
>             }
>             catch (BrokenBarrierException bb) {
>                 // OK, ignore
>                 bb.printStackTrace();
>             }
>             Thread.sleep(TIMEOUT);
>         }
>     }
>
>     static final class ReentrantLockLoop implements Runnable {
>         private int v = rng.nextInt();
>         private int completed;
>         private volatile int result = 17;
>         private final ReentrantLock lock = new ReentrantLock();
>         private final LoopHelpers.BarrierTimer timer = new
> LoopHelpers.BarrierTimer();
>         private final CyclicBarrier barrier;
>         private final int nthreads;
>
>         ReentrantLockLoop(int nthreads) {
>             this.nthreads = nthreads;
>             barrier = new CyclicBarrier(nthreads+1, timer);
>         }
>
>         void checkBarrierStatus() {
>             if(this.barrier.isBroken()) {
>                 throw new Error("barrier is broken !!!");
>             }
>         }
>
>         final void test() throws Exception {
>             checkBarrierStatus();
>
>             timer.run();
>
>             Thread[] threads = new Thread[nthreads];
>             for (int i = 0; i < threads.length; ++i) {
>                 threads[i] = new Thread(this, "th"+i);
>             }
>
>             for (int i = 0; i < threads.length; ++i) {
>                 threads[i].start();
>             }
>
>             Thread[] cancels =  (Thread[])(threads.clone());
>
>             Collections.shuffle(Arrays.asList(cancels), rng);
>             Thread.sleep(TIMEOUT);
>             for (int i = 0; i < cancels.length-2; ++i) {
>                 cancels[i].interrupt();
>                 // make sure all OK even when cancellations spaced out
>                 if ((i & 3) == 0) {
>                     Thread.sleep(1 + rng.nextInt(10));
>                 }
>             }
>
>             Thread.sleep(TIMEOUT);
>
>             try {
>                 // in here, the barrier might be broken because some
> working threads is interrupted.
>                 // so, catching the BrokenBarrierException to let
> testcase continue running
>                 barrier.await();
>             }
>             catch(BrokenBarrierException e) {
>                 // nop
>             }
>
>             if (print) {
>                 // since the barrier might be broken ( the barrier
> action will not run),
>                 // running here to ensure time is set.
>                 timer.run();
>                 long time = timer.getTime();
>                 double secs = (double)(time) / 1000000000.0;
>                 System.out.println("\t " + secs + "s run time");
>             }
>
>             int c;
>             lock.lock();
>             //System.out.println("main to verify completed.....");
>
>             try {
>                 c = completed;
>             }
>             finally {
>                 lock.unlock();
>             }
>             // here the completed should not < 2, it can not guarantee
> that always == 2.
>             if (c < 2) {
>                 throw new Error("Completed < 2 : " + c);
>             }
>
>             int r = result;
>             if (r == 0) { // avoid overoptimization
>                 System.out.println("useless result: " + r);
>             }
>         }
>
>         public final void run() {
>             try {
>                 checkBarrierStatus();
>
>                 int sum = v;
>                 int x = 0;
>                 int n = ITERS;
>                 do {
>                     try {
>                         lock.lockInterruptibly();
>                     }
>                     catch (InterruptedException ie) {
>                         break;
>                     }
>                     try {
>                         v = x = LoopHelpers.compute1(v);
>                     }
>                     finally {
>                         lock.unlock();
>                     }
>                     sum += LoopHelpers.compute2(x);
>                 } while (n-- > 0);
>
>                 /*
>                  * in the main thread, two threads are un-interrupted,
> others threads are interrupted.
>                  * this line, however, can not guarantee that only
> "un-interrupted" threads can go in,
>                  * but also interrupted threads can go in,
>                  * so, the "completed" will not always == 2, might be > 2
>                  */
>                 if (n < 0) {
>                     lock.lock();
>                     try {
>                         ++completed;
>                     }
>                     finally {
>                         lock.unlock();
>                     }
>                 }
>
>                 result += sum;
>                 barrier.await();
>             }
>             catch(BrokenBarrierException ex) {
>                 //ex.printStackTrace();
>             }
>             catch(InterruptedException ex) {
>                 //ex.printStackTrace();
>             }
>             catch (Exception ex) {
>                 ex.printStackTrace();
>                 return;
>             }
>         }
>     }
>
> }
>
>
> On Thu, Jan 5, 2017 at 12:20 PM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
>     Hi chenzero,
>
>     Attachments get stripped from the mailing lists so you'll need to
>     include your patch inline, or else get someone to host it for you on
>     cr.openjdk.java.net <http://cr.openjdk.java.net>
>
>     Thanks,
>     David
>     .....
>