Fixing wakeups caused by Thread.join() interacting with LockSupport.unpark.

This caused several concurrency tests to fail when run with timeouts, since
the tests cause park and unpark to happen frequently.

Also fixing a tiny issue with CoreTestRunnable to use the proper tmp directory
and to include the program of a failed external execution.

3 files changed, 108 insertions, 21 deletions

diff --git a/luni-kernel/src/main/java/java/lang/Thread.java b/luni-kernel/src/main/java/java/lang/Thread.java
index 3cde7e1..89d7ed6 100644
--- a/luni-kernel/src/main/java/java/lang/Thread.java
+++ b/luni-kernel/src/main/java/java/lang/Thread.java
@@ -73,6 +73,8 @@ import org.apache.harmony.security.fortress.SecurityUtils;
  */
 public class Thread implements Runnable {
 
+    private static final int NANOS_PER_MILLI = 1000000;
+
     /** Park states */
     private static class ParkState {
         /** park state indicating unparked */
@@ -973,7 +975,16 @@ public class Thread implements Runnable {
      * @since Android 1.0
      */
     public final void join() throws InterruptedException {
-        join(0, 0);
+        VMThread t = vmThread;
+        if (t == null) {
+            return;
+        }
+
+        synchronized (t) {
+            while (isAlive()) {
+                t.wait();
+            }
+        }
     }
 
     /**
@@ -1008,18 +1019,45 @@ public class Thread implements Runnable {
      * @since Android 1.0
      */
     public final void join(long millis, int nanos) throws InterruptedException {
-        if (millis < 0 || nanos < 0 || nanos > 999999) {
+        if (millis < 0 || nanos < 0 || nanos >= NANOS_PER_MILLI) {
             throw new IllegalArgumentException();
         }
 
-        VMThread t;
+        // avoid overflow: if total > 292,277 years, just wait forever
+        boolean overflow = millis >= (Long.MAX_VALUE - nanos) / NANOS_PER_MILLI;
+        boolean forever = (millis | nanos) == 0;
+        if (forever | overflow) {
+            join();
+            return;
+        }
 
-        t = this.vmThread;
+        VMThread t = vmThread;
+        if (t == null) {
+            return;
+        }
+
+        synchronized (t) {
+            if (!isAlive()) {
+                return;
+            }
+
+            // guaranteed not to overflow
+            long nanosToWait = millis * NANOS_PER_MILLI + nanos;
 
-        if (t != null) {
-            synchronized (t) {
-                if (isAlive())
-                    t.wait(millis, nanos);
+            // wait until this thread completes or the timeout has elapsed
+            long start = System.nanoTime();
+            while (true) {
+                t.wait(millis, nanos);
+                if (!isAlive()) {
+                    break;
+                }
+                long nanosElapsed = System.nanoTime() - start;
+                long nanosRemaining = nanosToWait - nanosElapsed;
+                if (nanosRemaining <= 0) {
+                    break;
+                }
+                millis = nanosRemaining / NANOS_PER_MILLI;
+                nanos = (int) (nanosRemaining - millis * NANOS_PER_MILLI);
             }
         }
     }
@@ -1491,8 +1529,8 @@ public class Thread implements Runnable {
                     break;
                 }
                 case ParkState.UNPARKED: {
-                    long millis = nanos / 1000000;
-                    nanos %= 1000000;
+                    long millis = nanos / NANOS_PER_MILLI;
+                    nanos %= NANOS_PER_MILLI;
 
                     parkState = ParkState.PARKED;
                     try {
@@ -1554,7 +1592,7 @@ public class Thread implements Runnable {
             if (delayMillis <= 0) {
                 parkState = ParkState.UNPARKED;
             } else {
-                parkFor(delayMillis * 1000000);
+                parkFor(delayMillis * NANOS_PER_MILLI);
             }
         }
     }
diff --git a/luni/src/test/java/com/google/coretests/CoreTestRunnable.java b/luni/src/test/java/com/google/coretests/CoreTestRunnable.java
index ab49e47..ed7797e 100644
--- a/luni/src/test/java/com/google/coretests/CoreTestRunnable.java
+++ b/luni/src/test/java/com/google/coretests/CoreTestRunnable.java
@@ -137,18 +137,18 @@ public class CoreTestRunnable implements Runnable {
         Throwable throwable = null;
         
         File file = File.createTempFile("isolation", ".tmp");
-        
-        fProcess = Runtime.getRuntime().exec(
-                (IS_DALVIK ? "dalvikvm" : "java") +
+
+        String program = (IS_DALVIK ? "dalvikvm" : "java") +
                 " -classpath " + System.getProperty("java.class.path") +
                 " -Djava.home=" + System.getProperty("java.home") +
                 " -Duser.home=" + System.getProperty("user.home") +
-                " -Djava.io.tmpdir=" + System.getProperty("user.home") +
+                " -Djava.io.tmpdir=" + System.getProperty("java.io.tmpdir") +
                 " com.google.coretests.CoreTestIsolator" +
                 " " + fTest.getClass().getName() +
                 " " + fTest.getName() +
-                " " + file.getAbsolutePath());
-        
+                " " + file.getAbsolutePath();
+        fProcess = Runtime.getRuntime().exec(program);
+
         int result = fProcess.waitFor();
         
         if (result != TestRunner.SUCCESS_EXIT) {
@@ -158,7 +158,7 @@ public class CoreTestRunnable implements Runnable {
                 throwable = (Throwable)ois.readObject();
                 ois.close();
             } catch (Exception ex) {
-                throwable = new RuntimeException("Error isolating test", ex);
+                throwable = new RuntimeException("Error isolating test: " + program, ex);
             }
         }
         
diff --git a/luni/src/test/java/org/apache/harmony/luni/tests/java/lang/ThreadTest.java b/luni/src/test/java/org/apache/harmony/luni/tests/java/lang/ThreadTest.java
index 47eb166..a24f457 100644
--- a/luni/src/test/java/org/apache/harmony/luni/tests/java/lang/ThreadTest.java
+++ b/luni/src/test/java/org/apache/harmony/luni/tests/java/lang/ThreadTest.java
@@ -23,9 +23,10 @@ import java.lang.Thread.UncaughtExceptionHandler;
 import java.security.Permission;
 import java.util.Map;
 import java.util.concurrent.Semaphore;
+import java.util.concurrent.locks.LockSupport;
+import java.util.concurrent.atomic.AtomicReference;
 
 import dalvik.annotation.AndroidOnly;
-import dalvik.annotation.KnownFailure;
 import dalvik.annotation.TestLevel;
 import dalvik.annotation.TestTargetClass;
 import dalvik.annotation.TestTargetNew;
@@ -913,7 +914,7 @@ public class ThreadTest extends junit.framework.TestCase {
         } catch (InterruptedException e) {
             fail("Join failed ");
         }
-        assertTrue("Joined thread is still alive", !st.isAlive());
+        assertFalse("Joined thread is still alive", st.isAlive());
         boolean result = true;
         Thread th = new Thread("test");
         try {
@@ -938,6 +939,44 @@ public class ThreadTest extends junit.framework.TestCase {
         st.start();        
     }
 
+    @TestTargetNew(
+            level = TestLevel.PARTIAL_COMPLETE,
+            notes = "Regression test for when join() failed due to spurious wakeups",
+            method = "join",
+            args = {long.class, int.class}
+    )
+    public void test_joinWithSpuriousInterruption() throws InterruptedException {
+        final Thread parker = new Thread() {
+            @Override
+            public void run() {
+                for (int i = 0; i < 10; i++) {
+                    // we used to get spurious wakeups upon unparking
+                    LockSupport.park();
+                }
+            }
+        };
+        Thread unparker = new Thread() {
+            @Override
+            public void run() {
+                for (int i = 0; i < 10; i++) {
+                    try {
+                        Thread.sleep(100);
+                        LockSupport.unpark(parker);
+                    } catch (InterruptedException ignored) {
+                    }
+                }
+            }
+        };
+
+        long startNanos = System.nanoTime();
+        parker.start();
+        unparker.start();
+        parker.join(500, 500000);
+        long netWaitTime = System.nanoTime() - startNanos;
+        assertTrue("Expected to wait at least 500000000ns, but was " + netWaitTime + "ns",
+                netWaitTime > 500000000);
+    }
+
     /**
      * @tests java.lang.Thread#join(long)
      */
@@ -1919,23 +1958,33 @@ public class ThreadTest extends junit.framework.TestCase {
             public void run() {
                   while (!sem.hasQueuedThreads()) {}
                   sem.release();
+
+                  // RUNNABLE
                   while (run) {}
+
                   try {
+                      // WAITING
                       sem.acquire();
                   } catch (InterruptedException e) {
                       fail("InterruptedException was thrown.");
                   }
+
+                  // BLOCKED
                   synchronized (lock) {
                       lock.equals(new Object());
                   }
                   synchronized (lock) {
                       try {
                         sem.release();
+
+                        // TIMED_WAITING
                         lock.wait(Long.MAX_VALUE);
                       } catch (InterruptedException e) {
                           // expected
                       }
                   }
+
+                  // TERMINATED upon return
             }
         };
         assertEquals(Thread.State.NEW, th.getState());
@@ -1976,7 +2025,7 @@ public class ThreadTest extends junit.framework.TestCase {
         }
         assertEquals(Thread.State.TERMINATED, th.getState());
     }
-    boolean run = true;
+    volatile boolean run = true;
     
     /**
      * @tests java.lang.Thread#getUncaughtExceptionHandler