[Core] Fix race condition in Timeout

core/src/main/java/cucumber/runtime/Timeout.java

@@ -1,8 +1,8 @@
 package cucumber.runtime;
 
 import java.util.concurrent.Executors;
-import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -14,29 +14,46 @@ private Timeout() {
     public static <T> T timeout(Callback<T> callback, long timeoutMillis) throws Throwable {
         if (timeoutMillis == 0) {
             return callback.call();
-        } else {
-            final Thread executionThread = Thread.currentThread();
-            final AtomicBoolean done = new AtomicBoolean();
+        }
+
+        /* We need to ensure a happens before relation exists between these events;
+         *   a. the timer setting the interrupt flag on the execution thread.
+         *   b. terminating and cleaning up the timer
+         * To do this we synchronize on monitor. The atomic boolean is merely a convenient container.
+         */
+        final Thread executionThread = Thread.currentThread();
+        final Object monitor = new Object();
+        final AtomicBoolean done = new AtomicBoolean();
 
-            ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();
-            ScheduledFuture<?> timer = executorService.schedule(new Runnable() {
-                @Override
-                public void run() {
+        ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();
+        ScheduledFuture<?> timer = executorService.schedule(new Runnable() {
+            @Override
+            public void run() {
+                synchronized (monitor) {
                     if (!done.get()) {
                         executionThread.interrupt();
                     }
                 }
-            }, timeoutMillis, TimeUnit.MILLISECONDS);
-            try {
-                return callback.call();
-            } catch (InterruptedException timeout) {
+            }
+        }, timeoutMillis, TimeUnit.MILLISECONDS);
+
+        try {
+            T result = callback.call();
+            // The callback may have been busy waiting.
+            if (Thread.interrupted()) {
                 throw new TimeoutException("Timed out after " + timeoutMillis + "ms.");
-            } finally {
+            }
+            return result;
+        } catch (InterruptedException timeout) {
+            throw new TimeoutException("Timed out after " + timeoutMillis + "ms.");
+        } finally {
+            synchronized (monitor) {
                 done.set(true);
                 timer.cancel(true);
                 executorService.shutdownNow();
+                // Clear the interrupted flag. It may have been set by the timer just before we returned the result.
+                Thread.interrupted();
             }
-
         }
     }
 

core/src/test/java/cucumber/runtime/TimeoutTest.java

@@ -7,6 +7,7 @@
 
 import static java.lang.Thread.sleep;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -61,6 +62,19 @@ public Void call() throws Throwable {
         fail();
     }
 
+
+    @Test(expected = TimeoutException.class)
+    public void times_out_busy_wait_if_it_takes_too_long() throws Throwable {
+        final Slow slow = new Slow();
+        Timeout.timeout(new Timeout.Callback<Void>() {
+            @Override
+            public Void call() throws Throwable {
+                slow.busyWait();
+                return null;
+            }
+        }, 1);
+    }
+
     @Test
     public void doesnt_leak_threads() throws Throwable {
 
@@ -88,6 +102,8 @@ public String call() throws Throwable {
     }
 
     public static class Slow {
+        int busyCounter = Integer.MIN_VALUE;
+
         public String slow(int millis) throws InterruptedException {
             sleep(millis);
             return String.format("slept %sms", millis);
@@ -102,5 +118,13 @@ public void infinite() throws InterruptedException {
         public void infiniteLatchWait() throws InterruptedException {
             new CountDownLatch(1).await();
         }
+
+        public int busyWait() throws InterruptedException {
+            while (busyCounter < Integer.MAX_VALUE) {
+                busyCounter += 1;
+            }
+
+            return busyCounter;
+        }
     }
 }