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
enso-org · Oct 7, 2024 · 9429a45 · 9429a45
1 parent a4f800d
commit 9429a45
Show file tree

Hide file tree

Showing 5 changed files with 204 additions and 12 deletions.
diff --git a/...trument-common/src/main/java/org/enso/interpreter/instrument/job/VisualizationResult.java b/...trument-common/src/main/java/org/enso/interpreter/instrument/job/VisualizationResult.java
@@ -43,9 +43,13 @@ public static String findExceptionMessage(Throwable ex) {
     }
   }
 
-  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) {

diff --git a/...-common/src/main/scala/org/enso/interpreter/instrument/execution/JobExecutionEngine.scala b/...-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)

diff --git a/...nt-common/src/main/scala/org/enso/interpreter/instrument/execution/ReentrantLocking.scala b/...nt-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 =>

diff --git a/...t-common/src/main/scala/org/enso/interpreter/instrument/job/ProgramExecutionSupport.scala b/...t-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,

diff --git a/...-tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeAsyncCommandsTest.scala b/...-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"))
+  }
+
 }