From ebe039ecebb0d00d6ba57a1832426e7f1ea48170 Mon Sep 17 00:00:00 2001
From: "M.P. Korstanje" <rien.korstanje@gmail.com>
Date: Wed, 11 Oct 2017 17:24:03 +0200
Subject: [PATCH] Fix race condition in Timeout

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.

Additionally Timeout did not throw any exceptions when the callback
was busy waiting but did set the interrupted flag. To resolve this
we check and throw after the completion of the callback.

This fixes #1241
---
 .../main/java/cucumber/runtime/Timeout.java   | 45 +++++++++++++------
 .../java/cucumber/runtime/TimeoutTest.java    | 24 ++++++++++
 2 files changed, 55 insertions(+), 14 deletions(-)

diff --git a/core/src/main/java/cucumber/runtime/Timeout.java b/core/src/main/java/cucumber/runtime/Timeout.java
index ae890e7794..2bc310edfc 100644
--- a/core/src/main/java/cucumber/runtime/Timeout.java
+++ b/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();
             }
-
         }
     }
 
diff --git a/core/src/test/java/cucumber/runtime/TimeoutTest.java b/core/src/test/java/cucumber/runtime/TimeoutTest.java
index a7d92ebf09..edc127a567 100644
--- a/core/src/test/java/cucumber/runtime/TimeoutTest.java
+++ b/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;
+        }
     }
 }