Fixed sporadic problems with timeouts on Win Py2.6.

pekkaklarck · pekkaklarck · commit 1a5eeaa7f58a · 2015-12-20T01:25:25.000+02:00
Timeout tests started to fail sporadically on Windows with Python 2.6 after recent refactoring of keyword execution logic. Based on git bisect the "bad commit" was 08390eb. The actual problem turned out to be in the code related to asynchronously raising exception in the runner thread to signal timeout. In some cases the exception was raised so that it occurred during the code that was trying to cancel it. Most likely the old keyword execution code catched the resulting TimeoutError but that didn't anymore happen after the refactoring. Not sure why that problem only manifested itself with Python 2.6, though. The fix has two parts: - Check has timeout occurred inside try/finally and wait for it to be actually raised. This ensures it won't happen inside the finally block. - Add new `_finished` flag to signal execution has finished and timeout should not be raised. Also use locks to avoid race conditions when setting `_finished` and `_timeout_occurred`. Interestingly the problem seems to vanish with just the first fix. Looking at the code, the problem could occur also then, so it is safer to keep both parts.
diff --git a/src/robot/running/timeouts/windows.py b/src/robot/running/timeouts/windows.py
@@ -14,45 +14,55 @@
 
 import ctypes
 import time
-from threading import Timer, current_thread
+from threading import current_thread, Lock, Timer
 
 from robot.errors import TimeoutError
 from robot.utils import py2to3
 
 
 class Timeout(object):
 
-    def __init__(self, timeout, timeout_error):
+    def __init__(self, timeout, error):
         self._runner_thread_id = current_thread().ident
-        self._timeout_error = self._create_timeout_error_class(timeout_error)
+        self._timeout_error = self._create_timeout_error_class(error)
         self._timer = Timer(timeout, self._raise_timeout_error)
         self._timeout_occurred = False
+        self._finished = False
+        self._lock = Lock()
 
-    def _create_timeout_error_class(self, timeout_error):
+    def _create_timeout_error_class(self, error):
         return py2to3(type(TimeoutError.__name__, (TimeoutError,),
-                           {'__unicode__': lambda self: timeout_error}))
+                           {'__unicode__': lambda self: error}))
 
     def execute(self, runnable):
-        self._start_timer()
         try:
-            return runnable()
+            self._start_timer()
+            result = runnable()
+            self._cancel_timer()
+            self._wait_for_raised_timeout()
+            return result
         finally:
-            self._stop_timer()
+            if self._timeout_occurred:
+                raise self._timeout_error()
 
     def _start_timer(self):
         self._timer.start()
 
-    def _stop_timer(self):
-        self._timer.cancel()
-        # In case timeout has occurred but the exception has not yet been
-        # thrown we need to do this to ensure that the exception
-        # is not thrown in an unsafe location
+    def _cancel_timer(self):
+        with self._lock:
+            self._finished = True
+            self._timer.cancel()
+
+    def _wait_for_raised_timeout(self):
         if self._timeout_occurred:
-            self._cancel_exception()
-            raise self._timeout_error()
+            while True:
+                time.sleep(0)
 
     def _raise_timeout_error(self):
-        self._timeout_occurred = True
+        with self._lock:
+            if self._finished:
+                return
+            self._timeout_occurred = True
         return_code = self._try_to_raise_timeout_error_in_runner_thread()
         # return code tells how many threads have been influenced
         while return_code > 1: # if more than one then cancel and retry
@@ -62,8 +72,9 @@ def _raise_timeout_error(self):
 
     def _try_to_raise_timeout_error_in_runner_thread(self):
         return ctypes.pythonapi.PyThreadState_SetAsyncExc(
-            self._runner_thread_id,
+            ctypes.c_long(self._runner_thread_id),
             ctypes.py_object(self._timeout_error))
 
     def _cancel_exception(self):
-        ctypes.pythonapi.PyThreadState_SetAsyncExc(self._runner_thread_id, None)
+        ctypes.pythonapi.PyThreadState_SetAsyncExc(
+            ctypes.c_long(self._runner_thread_id), None)
