Do not run visualizations on InterruptedException (#11250)

* Do not run visualizations on InterruptException There is no point in running visualization for the expression value that is InterruptedException. The latter is likely to bubble up the exception or create one that will be confusing to the user. Closes #11243 and partially addresses some of the symptomes of #11084. * Add a test for confusing visualization failures Previously a visualization failure would be reported: ``` Method `+` of type sleep interrupted could not be found. ``` * PR review Nit (cherry picked from commit 9429a4540a)
2024-11-22 03:32:23 +03:00 · 2024-10-07 12:29:17 +02:00 · 2024-10-07 12:29:17 +02:00 · 1a0ae7c84d
commit 1a0ae7c84d
parent 26854e2267
5 changed files with 204 additions and 12 deletions
--- a/engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/job/VisualizationResult.java
+++ b/engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/job/VisualizationResult.java
@ -43,9 +43,13 @@ public final class VisualizationResult {
    }
  }

-  public static boolean isInterruptedException(Throwable ex) {
-    var iop = InteropLibrary.getUncached();
-    return isInterruptedException(ex, iop);
+  public static boolean isInterruptedException(Object object) {
+    if (object instanceof Throwable ex) {
+      var iop = InteropLibrary.getUncached();
+      return isInterruptedException(ex, iop);
+    } else {
+      return false;
+    }
  }

  private static boolean isInterruptedException(Object ex, InteropLibrary iop) {
--- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/JobExecutionEngine.scala
+++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/JobExecutionEngine.scala
@ -190,7 +190,7 @@ final class JobExecutionEngine(
    logger.log(
      Level.FINE,
      "Aborting {0} jobs because {1}: {2}",
-      Array(cancellableJobs.length, reason, cancellableJobs.map(_.id))
+      Array[Any](cancellableJobs.length, reason, cancellableJobs.map(_.id))
    )
    cancellableJobs.foreach { runningJob =>
      runningJob.future.cancel(runningJob.job.mayInterruptIfRunning)
@ -215,7 +215,7 @@ final class JobExecutionEngine(
        logger.log(
          Level.FINE,
          "Aborting job {0} because {1}",
-          Array(runningJob.id, reason)
+          Array[Any](runningJob.id, reason)
        )
        runningJob.future.cancel(runningJob.job.mayInterruptIfRunning)
      }
@ -237,7 +237,7 @@ final class JobExecutionEngine(
        logger.log(
          Level.FINE,
          "Aborting job {0} because {1}",
-          Array(runningJob.id, reason)
+          Array[Any](runningJob.id, reason)
        )
        runningJob.future.cancel(runningJob.job.mayInterruptIfRunning)
      }
@ -260,7 +260,7 @@ final class JobExecutionEngine(
    logger.log(
      Level.FINE,
      "Aborting {0} background jobs because {1}: {2}",
-      Array(cancellableJobs.length, reason, cancellableJobs.map(_.id))
+      Array[Any](cancellableJobs.length, reason, cancellableJobs.map(_.id))
    )
    cancellableJobs.foreach { runningJob =>
      runningJob.future.cancel(runningJob.job.mayInterruptIfRunning)
--- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/ReentrantLocking.scala
+++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/ReentrantLocking.scala
@ -213,7 +213,7 @@ class ReentrantLocking(logger: TruffleLogger) extends Locking {
        contextLock.lock,
        "context lock",
        where
-      ) //acquireContextLock(contextId)
+      )
      callable.call()
    } catch {
      case _: InterruptedException =>
--- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/ProgramExecutionSupport.scala
+++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/ProgramExecutionSupport.scala
@ -545,7 +545,7 @@ object ProgramExecutionSupport {
        } else {
          runtimeCache.getAnyValue(visualization.expressionId)
        }
-        if (v != null) {
+        if (v != null && !VisualizationResult.isInterruptedException(v)) {
          executeAndSendVisualizationUpdate(
            contextId,
            runtimeCache,
--- a/engine/runtime-integration-tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeAsyncCommandsTest.scala
+++ b/engine/runtime-integration-tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeAsyncCommandsTest.scala
@ -6,7 +6,11 @@ import org.enso.common.RuntimeOptions
 import org.enso.interpreter.runtime.`type`.ConstantsGen
 import org.enso.polyglot.RuntimeServerInfo
 import org.enso.polyglot.runtime.Runtime.Api
-import org.enso.polyglot.runtime.Runtime.Api.{MethodCall, MethodPointer}
+import org.enso.polyglot.runtime.Runtime.Api.{
+  InvalidatedExpressions,
+  MethodCall,
+  MethodPointer
+}
 import org.enso.text.{ContentVersion, Sha3_224VersionCalculator}
 import org.graalvm.polyglot.Context
 import org.scalatest.BeforeAndAfterEach
@ -28,6 +32,19 @@ class RuntimeAsyncCommandsTest

  var context: TestContext = _

+  object Visualization {
+
+    val metadata = new Metadata
+
+    val code =
+      metadata.appendToCode(
+        """
+          |encode x = (x + 1).to_text
+          |""".stripMargin.linesIterator.mkString("\n")
+      )
+
+  }
+
  class TestContext(packageName: String)
      extends InstrumentTestContext(packageName) {
    val out: ByteArrayOutputStream = new ByteArrayOutputStream()
@ -245,7 +262,7 @@ class RuntimeAsyncCommandsTest
    diagnostic.stack should not be empty
  }

-  it should "interrupt running execution context without raising Panic" in {
+  it should "interrupt running execution context without sending Panic in expression updates" in {
    val moduleName = "Enso_Test.Test.Main"
    val contextId  = UUID.randomUUID()
    val requestId  = UUID.randomUUID()
@ -303,7 +320,7 @@ class RuntimeAsyncCommandsTest
    var iteration        = 0
    while (!isProgramStarted && iteration < 100) {
      val out = context.consumeOut
-      Thread.sleep(200)
+      Thread.sleep(100)
      isProgramStarted = out == List("started")
      iteration += 1
    }
@ -335,4 +352,175 @@ class RuntimeAsyncCommandsTest
      context.executionComplete(contextId)
    )
  }
+
+  it should "interrupt running execution context without sending Panic in visualization updates" in {
+    val contextId       = UUID.randomUUID()
+    val requestId       = UUID.randomUUID()
+    val visualizationId = UUID.randomUUID()
+    val moduleName      = "Enso_Test.Test.Main"
+    val metadata        = new Metadata("import Standard.Base.Data.Numbers\n\n")
+
+    val visualizationFile =
+      context.writeInSrcDir("Visualization", Visualization.code)
+
+    val idOp1 = metadata.addItem(203, 7)
+    val idOp2 = metadata.addItem(227, 13)
+
+    val code =
+      """from Standard.Base import all
+        |
+        |polyglot java import java.lang.Thread
+        |
+        |loop n s=0 =
+        |    if (s > n) then s else
+        |        Thread.sleep 200
+        |        loop n s+1
+        |
+        |main =
+        |    IO.println "started"
+        |    operator1 = loop 50
+        |    operator2 = operator1 + 1
+        |    operator2
+        |
+        |fun1 x = x.to_text
+        |""".stripMargin.linesIterator.mkString("\n")
+    val contents = metadata.appendToCode(code)
+    val mainFile = context.writeMain(contents)
+
+    // create context
+    context.send(Api.Request(requestId, Api.CreateContextRequest(contextId)))
+    context.receive shouldEqual Some(
+      Api.Response(requestId, Api.CreateContextResponse(contextId))
+    )
+
+    // Open visualizations
+    context.send(
+      Api.Request(
+        requestId,
+        Api.OpenFileRequest(
+          visualizationFile,
+          Visualization.code
+        )
+      )
+    )
+    context.receive shouldEqual Some(
+      Api.Response(Some(requestId), Api.OpenFileResponse)
+    )
+
+    // Open the new file
+    context.send(
+      Api.Request(requestId, Api.OpenFileRequest(mainFile, contents))
+    )
+    context.receive shouldEqual Some(
+      Api.Response(Some(requestId), Api.OpenFileResponse)
+    )
+
+    // push main
+    val item1 = Api.StackItem.ExplicitCall(
+      Api.MethodPointer(moduleName, moduleName, "main"),
+      None,
+      Vector()
+    )
+    context.send(
+      Api.Request(requestId, Api.PushContextRequest(contextId, item1))
+    )
+
+    // attach visualizations to both expressions
+    context.send(
+      Api.Request(
+        requestId,
+        Api.AttachVisualization(
+          visualizationId,
+          idOp2,
+          Api.VisualizationConfiguration(
+            contextId,
+            Api.VisualizationExpression.Text(
+              "Enso_Test.Test.Visualization",
+              "x -> encode x",
+              Vector()
+            ),
+            "Enso_Test.Test.Visualization"
+          )
+        )
+      )
+    )
+    context.send(
+      Api.Request(
+        requestId,
+        Api.AttachVisualization(
+          visualizationId,
+          idOp1,
+          Api.VisualizationConfiguration(
+            contextId,
+            Api.VisualizationExpression.Text(
+              "Enso_Test.Test.Visualization",
+              "x -> encode x",
+              Vector()
+            ),
+            "Enso_Test.Test.Visualization"
+          )
+        )
+      )
+    )
+
+    val response1 = context.receiveNIgnoreExpressionUpdates(
+      6,
+      timeoutSeconds = 20
+    )
+    response1 should contain allOf (
+      Api.Response(requestId, Api.PushContextResponse(contextId)),
+      Api.Response(requestId, Api.VisualizationAttached()),
+      context.executionComplete(contextId)
+    )
+    context.consumeOut
+    response1
+      .map(_.payload)
+      .count(_.isInstanceOf[Api.VisualizationAttached]) should be(2)
+    response1
+      .map(_.payload)
+      .count(_.isInstanceOf[Api.VisualizationUpdate]) should be(2)
+
+    context.send(
+      Api.Request(
+        requestId,
+        Api.RecomputeContextRequest(
+          contextId,
+          Some(InvalidatedExpressions.Expressions(Vector(idOp1, idOp2))),
+          None
+        )
+      )
+    )
+    var isProgramStarted = false
+    var iteration        = 0
+    while (!isProgramStarted && iteration < 100) {
+      val out = context.consumeOut
+      Thread.sleep(100)
+      isProgramStarted = out == List("started")
+      iteration += 1
+    }
+    if (!isProgramStarted) {
+      fail("Program start timed out")
+    }
+
+    // Trigger interruption
+    context.send(
+      Api.Request(requestId, Api.InterruptContextRequest(contextId))
+    )
+    val response2 = context.receiveNIgnoreExpressionUpdates(
+      5,
+      timeoutSeconds = 20
+    )
+    response2 should contain allOf (
+      Api.Response(requestId, Api.RecomputeContextResponse(contextId)),
+      Api.Response(requestId, Api.InterruptContextResponse(contextId)),
+      context.executionComplete(contextId)
+    )
+
+    val failure = response2.collectFirst({
+      case Api.Response(None, Api.VisualizationEvaluationFailed(_, msg, _)) =>
+        msg
+    })
+    failure should be(Symbol("empty"))
+  }
+
 }