Fix Deadlock on ProgressReport (Fixes #151) (#152)

commons/src/main/java/org/sonarsource/analyzer/commons/ProgressReport.java

@@ -20,6 +20,7 @@
 package org.sonarsource.analyzer.commons;
 
 import java.util.Iterator;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.stream.StreamSupport;
 import org.sonar.api.utils.log.Logger;
 import org.sonar.api.utils.log.Loggers;
@@ -36,7 +37,18 @@ public class ProgressReport implements Runnable {
   private final String adjective;
   private boolean success = false;
 
+  /**
+   * The report loop can not rely only on Thread.interrupted() to end, according to
+   * interrupted() javadoc, a thread interruption can be ignored because a thread was
+   * not alive at the time of the interrupt. This could happen if stop() is being called
+   * before ProgressReport's thread becomes alive.
+   * So this boolean flag ensures that ProgressReport never enter an infinite loop when
+   * Thread.interrupted() failed to be set to true.
+   */
+  private final AtomicBoolean interrupted = new AtomicBoolean();
+
   public ProgressReport(String threadName, long period, Logger logger, String adjective) {
+    interrupted.set(false);
     this.period = period;
     this.logger = logger;
     this.adjective = adjective;
@@ -56,11 +68,12 @@ public ProgressReport(String threadName, long period) {
   @Override
   public void run() {
     log(count + " source " + pluralizeFile(count) + " to be " + adjective);
-    while (!Thread.interrupted()) {
+    while (!(interrupted.get() || Thread.currentThread().isInterrupted())) {
       try {
         Thread.sleep(period);
         log(currentFileNumber + "/" + count + " " + pluralizeFile(currentFileNumber) + " " + adjective + ", current file: " + currentFilename);
       } catch (InterruptedException e) {
+        interrupted.set(true);
         thread.interrupt();
         break;
       }
@@ -101,12 +114,14 @@ public synchronized void nextFile() {
   }
 
   public synchronized void stop() {
+    interrupted.set(true);
     success = true;
     thread.interrupt();
     join();
   }
 
   public synchronized void cancel() {
+    interrupted.set(true);
     thread.interrupt();
     join();
   }

commons/src/test/java/org/sonarsource/analyzer/commons/ProgressReportTest.java

@@ -20,8 +20,10 @@
 package org.sonarsource.analyzer.commons;
 
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
 import org.junit.Rule;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
@@ -155,6 +157,40 @@ public void testStopPreserveTheInterruptedFlag() throws InterruptedException {
     assertThat(messages).contains("1/1" + " source file has been analyzed");
   }
 
+  @Test(timeout = 1000)
+  public void interrupting_the_thread_should_never_create_a_deadlock() {
+    ProgressReport report = new ProgressReport(ProgressReport.class.getName(), 500);
+
+    long start = System.currentTimeMillis();
+    report.start(Collections.emptyList());
+    report.stop();
+    long end = System.currentTimeMillis();
+
+    // stopping the report too soon could fail to interrupt the thread that was not yet alive,
+    // and fail to set the proper state for Thread.interrupted()
+    // this test ensures that the report does not loop once or is interrupted when stop() is
+    // called just after start()
+    assertThat(end - start).isLessThan(300);
+  }
+
+  @Test(timeout = 1000)
+  public void interrupted_thread_should_exit_immediately() throws InterruptedException {
+    ProgressReport report = new ProgressReport(ProgressReport.class.getName(), 500);
+    AtomicLong time = new AtomicLong(10000);
+    Thread selfInterruptedThread = new Thread(() -> {
+      // set the thread as interrupted
+      Thread.currentThread().interrupt();
+      long start = System.currentTimeMillis();
+      // execute run, while the thread is interrupted
+      report.run();
+      long end = System.currentTimeMillis();
+      time.set(end - start);
+    });
+    selfInterruptedThread.start();
+    selfInterruptedThread.join();
+    assertThat(time.get()).isLessThan(300);
+  }
+
   private static void waitForMessage(Logger logger) throws InterruptedException {
     synchronized (logger) {
       logger.wait();