From 3a98e8d2591652d696c08ef4b38659ca45ee4654 Mon Sep 17 00:00:00 2001
From: IlyaMuravjov <muravjovilya@gmail.com>
Date: Mon, 14 Aug 2023 17:00:40 +0300
Subject: [PATCH] Make `ThreadBasedExecutor` avoid race condition and not call
 `Thread.stop()` during while executing clean up callback

---
 .../kotlin/org/utbot/common/ThreadUtil.kt     | 55 ++++++++++++++-----
 .../execution/phases/PhasesController.kt      |  3 +-
 2 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/utbot-core/src/main/kotlin/org/utbot/common/ThreadUtil.kt b/utbot-core/src/main/kotlin/org/utbot/common/ThreadUtil.kt
index a4679f303d..1aee3398bd 100644
--- a/utbot-core/src/main/kotlin/org/utbot/common/ThreadUtil.kt
+++ b/utbot-core/src/main/kotlin/org/utbot/common/ThreadUtil.kt
@@ -1,10 +1,12 @@
 package org.utbot.common
 
-import java.util.WeakHashMap
 import java.util.concurrent.ArrayBlockingQueue
 import java.util.concurrent.TimeUnit
+import java.util.concurrent.locks.ReentrantLock
 import kotlin.concurrent.thread
+import kotlin.concurrent.withLock
 import kotlin.properties.ReadOnlyProperty
+import kotlin.random.Random
 import kotlin.reflect.KProperty
 
 
@@ -25,15 +27,34 @@ class ThreadBasedExecutor {
         val threadLocal by threadLocalLazy { ThreadBasedExecutor() }
     }
 
-    // there's no `WeakHashSet`, so we use `WeakHashMap` with dummy values
-    private val timedOutThreads = WeakHashMap<Thread, Unit>()
+    /**
+     * Used to avoid calling [Thread.stop] during clean up.
+     *
+     * @see runCleanUpIfTimedOut
+     */
+    private val timeOutCleanUpLock = ReentrantLock()
+
+    /**
+     * `null` when either:
+     * - no tasks have yet been run
+     * - current task timed out, and we are waiting for its thread to die
+     */
+    @Volatile
     private var thread: Thread? = null
 
     private var requestQueue = ArrayBlockingQueue<() -> Any?>(1)
     private var responseQueue = ArrayBlockingQueue<Result<Any?>>(1)
 
-    fun isCurrentThreadTimedOut(): Boolean =
-        Thread.currentThread() in timedOutThreads
+    /**
+     * Can be called from lambda passed to [invokeWithTimeout].
+     * [ThreadBasedExecutor] guarantees that it won't attempt to terminate [cleanUpBlock] with [Thread.stop].
+     */
+    fun runCleanUpIfTimedOut(cleanUpBlock: () -> Unit) {
+        timeOutCleanUpLock.withLock {
+            if (thread == null)
+                cleanUpBlock()
+        }
+    }
 
     /**
      * Invoke [action] with timeout.
@@ -64,16 +85,22 @@ class ThreadBasedExecutor {
         if (res == null) {
             try {
                 val t = thread ?: return res
-                timedOutThreads[t] = Unit
+                thread = null
                 t.interrupt()
                 t.join(10)
-                if (t.isAlive)
-                    @Suppress("DEPRECATION")
-                    t.stop()
+                // to avoid race condition we need to wait for `t` to die
+                while (t.isAlive) {
+                    timeOutCleanUpLock.withLock {
+                        @Suppress("DEPRECATION")
+                        t.stop()
+                    }
+                    // If somebody catches `ThreadDeath`, for now we
+                    // just wait for at most 10s and throw another one.
+                    //
+                    // A better approach may be to kill instrumented process.
+                    t.join(10_000)
+                }
             } catch (_: Throwable) {}
-
-            thread = null
-
         }
         return res
     }
@@ -90,9 +117,9 @@ class ThreadBasedExecutor {
             requestQueue = ArrayBlockingQueue<() -> Any?>(1)
             responseQueue = ArrayBlockingQueue<Result<Any?>>(1)
 
-            thread = thread(name = "executor", isDaemon = true) {
+            thread = thread(name = "executor @${Random.nextInt(10_000)}", isDaemon = true) {
                 try {
-                    while (true) {
+                    while (thread === Thread.currentThread()) {
                         val next = requestQueue.take()
                         responseQueue.offer(kotlin.runCatching { next() })
                     }
diff --git a/utbot-instrumentation/src/main/kotlin/org/utbot/instrumentation/instrumentation/execution/phases/PhasesController.kt b/utbot-instrumentation/src/main/kotlin/org/utbot/instrumentation/instrumentation/execution/phases/PhasesController.kt
index 0a2a0bab70..45e3cebc3c 100644
--- a/utbot-instrumentation/src/main/kotlin/org/utbot/instrumentation/instrumentation/execution/phases/PhasesController.kt
+++ b/utbot-instrumentation/src/main/kotlin/org/utbot/instrumentation/instrumentation/execution/phases/PhasesController.kt
@@ -72,8 +72,9 @@ class PhasesController(
                 try {
                     phase.block()
                 } finally {
-                    if (executor.isCurrentThreadTimedOut())
+                    executor.runCleanUpIfTimedOut {
                         instrumentationContext.onPhaseTimeout(phase)
+                    }
                 }
             }
         } ?: throw TimeoutException("Timeout $timeoutForCurrentPhase ms for phase ${phase.javaClass.simpleName} elapsed, controller timeout - $timeout")