From c6ceee34a7ed5304cd82e1ea317cc22b382f60f8 Mon Sep 17 00:00:00 2001
From: dgelessus <dgelessus@users.noreply.github.com>
Date: Thu, 17 Sep 2020 19:05:30 +0200
Subject: [PATCH] Run commands in a separate executor instead of the main
kernel thread
This fixes a bug where interrupting a command would terminate the main
event loop thread, causing the kernel to stop responding. This happened
because the interrupt handling sent a Java interrupt to the thread
executing the current command, which was always the main event loop
thread. Now commands are executed on a separate executor, which can
safely have Java interrupts sent to it without affecting any
jupyter-jvm-basekernel threads.
---
CHANGELOG.md | 1 +
.../java/de/prob2/jupyter/ProBKernel.java | 45 ++++++++++++++-----
.../prob2/jupyter/commands/TimeCommand.java | 2 +-
3 files changed, 37 insertions(+), 11 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 525292e..d26e051 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,7 @@
* Significantly refactored the logic for parsing commands and their arguments.
* This is an internal change and should not affect any user-visible behavior. That is, all inputs that were accepted by previous versions should still be accepted - if any previously valid inputs are no longer accepted, this is a bug.
* As a side effect, the inspection and code completion features now work better in a few edge cases.
+* Fixed a bug where interrupting a command could make the kernel completely stop responding, requiring a manual restart.
## [1.2.0](https://www3.hhu.de/stups/downloads/prob2-jupyter/prob2-jupyter-kernel-1.2.0-all.jar)
diff --git a/src/main/java/de/prob2/jupyter/ProBKernel.java b/src/main/java/de/prob2/jupyter/ProBKernel.java
index 3ccfae2..66ffbd6 100644
--- a/src/main/java/de/prob2/jupyter/ProBKernel.java
+++ b/src/main/java/de/prob2/jupyter/ProBKernel.java
@@ -16,6 +16,11 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Properties;
+import java.util.concurrent.CancellationException;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Function;
import java.util.regex.Matcher;
@@ -23,6 +28,7 @@ import java.util.regex.Pattern;
import java.util.stream.Collectors;
import com.google.common.base.MoreObjects;
+import com.google.common.util.concurrent.Futures;
import com.google.inject.Inject;
import com.google.inject.Injector;
import com.google.inject.Singleton;
@@ -231,7 +237,8 @@ public final class ProBKernel extends BaseKernel {
private final @NotNull ReusableAnimator animator;
private final @NotNull Map<@NotNull String, @NotNull Command> commands;
- private final @NotNull AtomicReference<@Nullable Thread> currentEvalThread;
+ private final @NotNull ExecutorService commandExecutor;
+ private final @NotNull AtomicReference<@NotNull Future<@Nullable DisplayData>> currentCommandFuture;
private final @NotNull Map<@NotNull String, @NotNull String> variables;
private @NotNull Path currentMachineDirectory;
@@ -251,7 +258,8 @@ public final class ProBKernel extends BaseKernel {
.map(injector::getInstance)
.collect(Collectors.toMap(Command::getName, cmd -> cmd));
- this.currentEvalThread = new AtomicReference<>(null);
+ this.commandExecutor = Executors.newSingleThreadExecutor(runnable -> new Thread(runnable, "Command Executor"));
+ this.currentCommandFuture = new AtomicReference<>(Futures.immediateCancelledFuture());
this.variables = new HashMap<>();
this.currentMachineDirectory = Paths.get("");
@@ -469,16 +477,35 @@ public final class ProBKernel extends BaseKernel {
return this.executeCommand(split.getName(), split.getArgString());
}
+ public @Nullable DisplayData evalOnCurrentThread(final @NotNull String expr) {
+ return this.evalInternal(new PositionedString(expr, 0));
+ }
+
@Override
public @Nullable DisplayData eval(final String expr) {
assert expr != null;
- this.currentEvalThread.set(Thread.currentThread());
+ final Future<DisplayData> oldFuture = this.currentCommandFuture.get();
+ assert oldFuture.isDone() || oldFuture.isCancelled();
+ final Future<DisplayData> future = this.commandExecutor.submit(() -> this.evalOnCurrentThread(expr));
+ this.currentCommandFuture.set(future);
try {
- return evalInternal(new PositionedString(expr, 0));
- } finally {
- this.currentEvalThread.set(null);
+ return future.get();
+ } catch (final InterruptedException e) {
+ LOGGER.error("Main kernel thread (event loop thread) interrupted while executing command", e);
+ Thread.currentThread().interrupt();
+ return null;
+ } catch (final CancellationException e) {
+ LOGGER.info("Command execution cancelled by user", e);
+ throw new UserErrorException("Interrupted by user");
+ } catch (final ExecutionException e) {
+ LOGGER.warn("Exception while executing command", e);
+ if (e.getCause() instanceof RuntimeException) {
+ throw (RuntimeException)e.getCause();
+ } else {
+ throw new IllegalStateException("Checked exception thrown by command, this should never happen", e);
+ }
}
}
@@ -612,14 +639,12 @@ public final class ProBKernel extends BaseKernel {
@Override
public void onShutdown(final boolean isRestarting) {
this.animationSelector.getCurrentTrace().getStateSpace().kill();
+ this.commandExecutor.shutdownNow();
}
@Override
public void interrupt() {
- final Thread evalThread = this.currentEvalThread.get();
- if (evalThread != null) {
- evalThread.interrupt();
- }
+ this.currentCommandFuture.get().cancel(true);
this.animationSelector.getCurrentTrace().getStateSpace().sendInterrupt();
}
diff --git a/src/main/java/de/prob2/jupyter/commands/TimeCommand.java b/src/main/java/de/prob2/jupyter/commands/TimeCommand.java
index a7cf5c9..7fb5552 100644
--- a/src/main/java/de/prob2/jupyter/commands/TimeCommand.java
+++ b/src/main/java/de/prob2/jupyter/commands/TimeCommand.java
@@ -63,7 +63,7 @@ public final class TimeCommand implements Command {
public @Nullable DisplayData run(final @NotNull ParsedArguments args) {
final ProBKernel kernel = this.injector.getInstance(ProBKernel.class);
final Stopwatch stopwatch = Stopwatch.createStarted();
- final DisplayData result = kernel.eval(args.get(COMMAND_AND_ARGS_PARAM));
+ final DisplayData result = kernel.evalOnCurrentThread(args.get(COMMAND_AND_ARGS_PARAM));
stopwatch.stop();
final Duration elapsed = stopwatch.elapsed();
final String text = String.format("Execution time: %d.%09d seconds", elapsed.get(ChronoUnit.SECONDS), elapsed.get(ChronoUnit.NANOS));
--
GitLab