Remove context locks from visualization commands (#7953)

It looks like visualization commands had required context lock unnecessairly. Context manager methods are synchronized and therefore should not need the lock before submitting a correspodning background job. Additionally, the presence of a context lock leads a deadlock: 1. Consider a long running execution of a program that does not finish within the 5 seconds 2. In the meantime there comes either an `AttachVisualization` or `DetachVisualization` request from the client The latter will get stuck and eventually timeout out because it cannot acquire the lock withing the required time limits. It is still possible that a single long-running `ExecuteJob` might block other ones (including visualization ones) but that's a separate work. Fixes some issues present in #7941. # Important Notes We need to still investigate `ExecuteJob`s need for context lock which might delay the execution of other background jobs that require it as well (mostly concerned about visualization).
2024-11-22 22:10:15 +03:00 · 2023-10-06 18:48:38 +02:00 · 2023-10-06 18:48:38 +02:00 · 16c8d2e302
commit 16c8d2e302
parent 3b6f13c7b9
11 changed files with 80 additions and 135 deletions
--- a/engine/language-server/src/main/scala/org/enso/languageserver/boot/MainModule.scala
+++ b/engine/language-server/src/main/scala/org/enso/languageserver/boot/MainModule.scala
@ -50,6 +50,7 @@ import org.enso.logger.akka.AkkaConverter
 import org.enso.polyglot.{HostAccessFactory, RuntimeOptions, RuntimeServerInfo}
 import org.enso.searcher.sql.{SqlDatabase, SqlSuggestionsRepo}
 import org.enso.text.{ContentBasedVersioning, Sha3_224VersionCalculator}
+import org.graalvm.polyglot.Engine
 import org.graalvm.polyglot.Context
 import org.graalvm.polyglot.io.MessageEndpoint
 import org.slf4j.event.Level
@ -319,7 +320,14 @@ class MainModule(serverConfig: LanguageServerConfig, logLevel: Level) {
        connection
      } else null
    })
-  if (RuntimeOptions.isEspressoEnabled()) {
+  if (
+    Engine
+      .newBuilder()
+      .allowExperimentalOptions(true)
+      .build
+      .getLanguages()
+      .containsKey("java")
+  ) {
    builder
      .option("java.ExposeNativeJavaVM", "true")
      .option("java.Polyglot", "true")
--- a/engine/polyglot-api/src/main/java/org/enso/polyglot/RuntimeOptions.java
+++ b/engine/polyglot-api/src/main/java/org/enso/polyglot/RuntimeOptions.java
@ -159,9 +159,4 @@ public class RuntimeOptions {
  private static String interpreterOptionName(String name) {
    return LanguageInfo.ID + ".interpreter." + name;
  }
-
-  /** True if experiemental Espresso support should be allowed. */
-  public static boolean isEspressoEnabled() {
-    return "espresso".equals(System.getenv("ENSO_JAVA"));
-  }
 }
--- a/engine/runner/src/main/scala/org/enso/runner/ContextFactory.scala
+++ b/engine/runner/src/main/scala/org/enso/runner/ContextFactory.scala
@ -7,6 +7,7 @@ import org.enso.polyglot.debugger.{
  DebuggerSessionManagerEndpoint
 }
 import org.enso.polyglot.{HostAccessFactory, PolyglotContext, RuntimeOptions}
+import org.graalvm.polyglot.Engine
 import org.graalvm.polyglot.Context
 import org.slf4j.event.Level

@ -109,7 +110,14 @@ class ContextFactory {
    if (graalpy.exists()) {
      builder.option("python.Executable", graalpy.getAbsolutePath());
    }
-    if (RuntimeOptions.isEspressoEnabled()) {
+    if (
+      Engine
+        .newBuilder()
+        .allowExperimentalOptions(true)
+        .build()
+        .getLanguages()
+        .containsKey("java")
+    ) {
      builder
        .option("java.ExposeNativeJavaVM", "true")
        .option("java.Polyglot", "true")
--- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/command/AttachVisualizationCmd.scala
+++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/command/AttachVisualizationCmd.scala
@ -3,8 +3,6 @@ package org.enso.interpreter.instrument.command
 import org.enso.interpreter.instrument.execution.RuntimeContext
 import org.enso.interpreter.instrument.job.{ExecuteJob, UpsertVisualizationJob}
 import org.enso.polyglot.runtime.Runtime.Api
-
-import java.util.logging.Level
 import scala.concurrent.{ExecutionContext, Future}

 /** A command that attaches a visualization to an expression.
@ -15,38 +13,12 @@ import scala.concurrent.{ExecutionContext, Future}
 class AttachVisualizationCmd(
  maybeRequestId: Option[Api.RequestId],
  request: Api.AttachVisualization
-) extends AsynchronousCommand(maybeRequestId) {
+) extends ContextCmd(
+      request.visualizationConfig.executionContextId,
+      maybeRequestId
+    ) {

-  /** @inheritdoc */
-  override def executeAsynchronously(implicit
-    ctx: RuntimeContext,
-    ec: ExecutionContext
-  ): Future[Unit] = {
-    val logger        = ctx.executionService.getLogger
-    val contextId     = request.visualizationConfig.executionContextId
-    val lockTimestamp = ctx.locking.acquireContextLock(contextId)
-    try {
-      if (doesContextExist) {
-        attachVisualization()
-      } else {
-        replyWithContextNotExistError()
-      }
-    } finally {
-      ctx.locking.releaseContextLock(contextId)
-      logger.log(
-        Level.FINEST,
-        s"Kept context lock [AttachVisualizationCmd] for ${System.currentTimeMillis() - lockTimestamp} milliseconds"
-      )
-    }
-  }
-
-  private def doesContextExist(implicit ctx: RuntimeContext): Boolean = {
-    ctx.contextManager.contains(
-      request.visualizationConfig.executionContextId
-    )
-  }
-
-  private def attachVisualization()(implicit
+  override protected def executeCmd()(implicit
    ctx: RuntimeContext,
    ec: ExecutionContext
  ): Future[Unit] = {
@ -69,17 +41,6 @@ class AttachVisualizationCmd(
    }
  }

-  private def replyWithContextNotExistError()(implicit
-    ctx: RuntimeContext,
-    ec: ExecutionContext
-  ): Future[Unit] = {
-    Future {
-      reply(
-        Api.ContextNotExistError(request.visualizationConfig.executionContextId)
-      )
-    }
-  }
-
  override def toString: String = {
    "AttachVisualizationCmd(visualizationId: " + request.visualizationId + ",expressionId=" + request.expressionId + ")"
  }
--- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/command/ContextCmd.scala
+++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/command/ContextCmd.scala
@ -0,0 +1,41 @@
+package org.enso.interpreter.instrument.command
+
+import org.enso.interpreter.instrument.execution.RuntimeContext
+import org.enso.polyglot.runtime.Runtime.Api
+import org.enso.polyglot.runtime.Runtime.Api.ContextId
+
+import scala.concurrent.{ExecutionContext, Future}
+
+abstract class ContextCmd(
+  contextId: ContextId,
+  maybeRequestId: Option[Api.RequestId]
+) extends AsynchronousCommand(maybeRequestId) {
+
+  protected def executeCmd()(implicit
+    ctx: RuntimeContext,
+    ec: ExecutionContext
+  ): Future[Unit]
+
+  override def executeAsynchronously(implicit
+    ctx: RuntimeContext,
+    ec: ExecutionContext
+  ): Future[Unit] = {
+    if (ctx.contextManager.contains(contextId)) {
+      executeCmd()
+    } else {
+      replyWithContextNotExistError()
+    }
+  }
+
+  protected def replyWithContextNotExistError()(implicit
+    ctx: RuntimeContext,
+    ec: ExecutionContext
+  ): Future[Unit] = {
+    Future {
+      reply(
+        Api.ContextNotExistError(contextId)
+      )
+    }
+  }
+
+}
--- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/command/DetachVisualizationCmd.scala
+++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/command/DetachVisualizationCmd.scala
@ -5,7 +5,6 @@ import org.enso.interpreter.instrument.job.DetachVisualizationJob
 import org.enso.polyglot.runtime.Runtime.Api
 import org.enso.polyglot.runtime.Runtime.Api.RequestId

-import java.util.logging.Level
 import scala.concurrent.{ExecutionContext, Future}

 /** A command that detaches a visualization from the expression.
@ -16,36 +15,11 @@ import scala.concurrent.{ExecutionContext, Future}
 class DetachVisualizationCmd(
  maybeRequestId: Option[RequestId],
  request: Api.DetachVisualization
-) extends AsynchronousCommand(maybeRequestId) {
+) extends ContextCmd(request.contextId, maybeRequestId) {

-  /** @inheritdoc */
-  override def executeAsynchronously(implicit
+  override protected def executeCmd()(implicit
    ctx: RuntimeContext,
    ec: ExecutionContext
-  ): Future[Unit] = {
-    val logger        = ctx.executionService.getLogger
-    val lockTimestamp = ctx.locking.acquireContextLock(request.contextId)
-    try {
-      if (doesContextExist) {
-        detachVisualization()
-      } else {
-        replyWithContextNotExistError()
-      }
-    } finally {
-      ctx.locking.releaseContextLock(request.contextId)
-      logger.log(
-        Level.FINEST,
-        s"Kept context lock [DetachVisualizationCmd] for ${System.currentTimeMillis() - lockTimestamp} milliseconds"
-      )
-    }
-  }
-
-  private def doesContextExist(implicit ctx: RuntimeContext): Boolean = {
-    ctx.contextManager.contains(request.contextId)
-  }
-
-  private def detachVisualization()(implicit
-    ctx: RuntimeContext
  ): Future[Unit] = {
    ctx.endpoint.sendToClient(
      Api.Response(maybeRequestId, Api.VisualizationDetached())
@ -59,13 +33,4 @@ class DetachVisualizationCmd(
    )
  }

-  private def replyWithContextNotExistError()(implicit
-    ctx: RuntimeContext,
-    ec: ExecutionContext
-  ): Future[Unit] = {
-    Future {
-      reply(Api.ContextNotExistError(request.contextId))
-    }
-  }
-
 }
--- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/command/ModifyVisualizationCmd.scala
+++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/command/ModifyVisualizationCmd.scala
@ -10,7 +10,6 @@ import org.enso.interpreter.instrument.job.{
 import org.enso.polyglot.runtime.Runtime.Api
 import org.enso.polyglot.runtime.Runtime.Api.ExpressionId

-import java.util.logging.Level
 import scala.concurrent.{ExecutionContext, Future}

 /** A command that modifies a visualization.
@ -21,32 +20,12 @@ import scala.concurrent.{ExecutionContext, Future}
 class ModifyVisualizationCmd(
  maybeRequestId: Option[Api.RequestId],
  request: Api.ModifyVisualization
-) extends AsynchronousCommand(maybeRequestId) {
+) extends ContextCmd(
+      request.visualizationConfig.executionContextId,
+      maybeRequestId
+    ) {

-  /** @inheritdoc */
-  override def executeAsynchronously(implicit
-    ctx: RuntimeContext,
-    ec: ExecutionContext
-  ): Future[Unit] = {
-    val logger        = ctx.executionService.getLogger
-    val contextId     = request.visualizationConfig.executionContextId
-    val lockTimestamp = ctx.locking.acquireContextLock(contextId)
-    try {
-      if (doesContextExist) {
-        modifyVisualization()
-      } else {
-        replyWithContextNotExistError()
-      }
-    } finally {
-      ctx.locking.releaseContextLock(contextId)
-      logger.log(
-        Level.FINEST,
-        s"Kept context lock [UpsertVisualizationJob] for ${System.currentTimeMillis() - lockTimestamp} milliseconds"
-      )
-    }
-  }
-
-  private def modifyVisualization()(implicit
+  override protected def executeCmd()(implicit
    ctx: RuntimeContext,
    ec: ExecutionContext
  ): Future[Unit] = {
@ -99,23 +78,6 @@ class ModifyVisualizationCmd(
    }
  }

-  private def doesContextExist(implicit ctx: RuntimeContext): Boolean = {
-    ctx.contextManager.contains(
-      request.visualizationConfig.executionContextId
-    )
-  }
-
-  private def replyWithContextNotExistError()(implicit
-    ctx: RuntimeContext,
-    ec: ExecutionContext
-  ): Future[Unit] = {
-    Future {
-      reply(
-        Api.ContextNotExistError(request.visualizationConfig.executionContextId)
-      )
-    }
-  }
-
  override def toString: String = {
    "ModifyVisualizationCmd(visualizationId: " + request.visualizationId + ")"
  }
--- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/ExecuteJob.scala
+++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/ExecuteJob.scala
@ -86,7 +86,7 @@ class ExecuteJob(
      ctx.locking.releaseContextLock(contextId)
      logger.log(
        Level.FINEST,
-        s"Kept context lock [ExecuteJob] for ${contextId} for ${System.currentTimeMillis() - acquiredLock}"
+        s"Kept context lock [ExecuteJob] for ${contextId} for ${System.currentTimeMillis() - acquiredLock} milliseconds"
      )

    }
--- a/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Module.scala
+++ b/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Module.scala
@ -18,7 +18,7 @@ import org.enso.compiler.core.ir.module.scope.{Definition, Export, Import}
  * @param diagnostics compiler diagnostics for this node
  */
@SerialVersionUID(
-  7681L // SuggestionBuilder needs to send ascribedType of constructor parameters
+  7953L // instrumentor
 )       // prevents reading broken caches, see PR-3692 for details
 sealed case class Module(
  imports: List[Import],
--- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/EnsoContext.java
+++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/EnsoContext.java
@ -520,7 +520,10 @@ public final class EnsoContext {
    }
    guestJava = null;
    var envJava = System.getenv("ENSO_JAVA");
-    if (RuntimeOptions.isEspressoEnabled()) {
+    if (envJava == null) {
+      return guestJava;
+    }
+    if ("espresso".equals(envJava)) {
      var src = Source.newBuilder("java", "<Bindings>", "getbindings.java").build();
      try {
        guestJava = environment.parsePublic(src).call();
@ -536,6 +539,8 @@ public final class EnsoContext {
          throw ise;
        }
      }
+    } else {
+      throw new IllegalStateException("Specify ENSO_JAVA=espresso to use Espresso. Was: " + envJava);
    }
    return guestJava;
  }
--- a/engine/runtime/src/main/scala/org/enso/compiler/data/BindingsMap.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/data/BindingsMap.scala
@ -22,7 +22,7 @@ import scala.annotation.unused
  */

@SerialVersionUID(
-  7681L // verify ascribed types
+  7953L // instrumentor
 )
 case class BindingsMap(
  definedEntities: List[DefinedEntity],