From 077497d9b8a2b19a7de2f4261ba02318885e1f55 Mon Sep 17 00:00:00 2001
From: Josh B <421772+HT154@users.noreply.github.com>
Date: Fri, 8 Nov 2024 13:18:13 -0800
Subject: [PATCH] Fix a possible deadlock during external reader process close
 (#786) (#787)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Fix a possible deadlock during external reader process close

* Apply spotless

---------

Co-authored-by: Philip K.F. Hölzenspies <holzensp@gmail.com>
---
 .../ExternalReaderProcessImpl.java            | 44 ++++++-------------
 1 file changed, 13 insertions(+), 31 deletions(-)

diff --git a/pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcessImpl.java b/pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcessImpl.java
index 7aec2a57..ec46c5f6 100644
--- a/pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcessImpl.java
+++ b/pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcessImpl.java
@@ -17,17 +17,16 @@ package org.pkl.core.externalreader;
 
 import java.io.IOException;
 import java.lang.ProcessBuilder.Redirect;
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Random;
-import java.util.Timer;
-import java.util.TimerTask;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
 import javax.annotation.concurrent.GuardedBy;
-import org.pkl.core.Duration;
 import org.pkl.core.evaluatorSettings.PklEvaluatorSettings.ExternalReader;
 import org.pkl.core.externalreader.ExternalReaderMessages.*;
 import org.pkl.core.messaging.MessageTransport;
@@ -152,40 +151,23 @@ final class ExternalReaderProcessImpl implements ExternalReaderProcess {
   @Override
   public void close() {
     synchronized (lock) {
+      if (closed) return;
       closed = true;
-      if (process == null || !process.isAlive()) {
-        return;
-      }
 
       try {
-        if (transport != null) {
+        if (transport != null && process != null && process.isAlive()) {
           transport.send(new CloseExternalProcess());
+          process.waitFor(CLOSE_TIMEOUT.toMillis(), TimeUnit.MILLISECONDS);
+        }
+      } catch (Exception ignored) {
+      } finally {
+        if (process != null) {
+          // no-op unless process is alive
+          process.destroyForcibly();
+        }
+        if (transport != null) {
           transport.close();
         }
-
-        // forcefully stop the process after the timeout
-        // note that both transport.close() and process.destroy() are safe to call multiple times
-        new Timer()
-            .schedule(
-                new TimerTask() {
-                  @Override
-                  public void run() {
-                    if (process != null) {
-                      transport.close();
-                      process.destroyForcibly();
-                    }
-                  }
-                },
-                CLOSE_TIMEOUT.inWholeMillis());
-
-        // block on process exit
-        process.onExit().get();
-      } catch (Exception e) {
-        transport.close();
-        process.destroyForcibly();
-      } finally {
-        process = null;
-        transport = null;
       }
     }
   }