From 0402e8bafbdd2bc0723cdf01d5198733277ea819 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Mon, 27 Nov 2023 15:01:23 +0100 Subject: [PATCH] Ensure compilation is run with a compilation lock (#8395) Evaluating visualization expression may trigger a full compilation. A change in #7042 went a bit too far and led to a situation when there could be compilations running at the same time leading to a rather obscure `RedefinedMethodException` when the compilation on one thread already finished. This will make the logic correct again at the price of potentially slowing the processing of visualization. Closes #8296. # Important Notes Should make visualizations a bit more stable as well. --- .../instrument/execution/Locking.scala | 4 ++++ .../execution/ReentrantLocking.scala | 6 +++++ .../instrument/job/EnsureCompiledJob.scala | 2 +- .../job/UpsertVisualizationJob.scala | 23 +++++++++---------- .../org/enso/interpreter/epb/EpbContext.java | 4 ++-- 5 files changed, 24 insertions(+), 15 deletions(-) diff --git a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/Locking.scala b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/Locking.scala index 1b184afa51d..7e72f86a609 100644 --- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/Locking.scala +++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/Locking.scala @@ -29,6 +29,10 @@ trait Locking { */ def releaseWriteCompilationLock(): Unit + /** Aseerts a compilation write lock is held by the current thread. + */ + def assertWriteCompilationLock(): Unit + /** Acquires a compilation read lock and returns a timestamp when it succeeded. * * @return timestamp of when the lock was acquired diff --git 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 index 8cb4b0b5c4a..e710bdddfe6 100644 --- 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 @@ -99,6 +99,12 @@ class ReentrantLocking(logger: TruffleLogger) extends Locking { override def releaseWriteCompilationLock(): Unit = compilationLock.writeLock().unlock() + override def assertWriteCompilationLock(): Unit = + assert( + compilationLock.writeLock().isHeldByCurrentThread, + "should already held write compilation lock during the operation" + ) + /** @inheritdoc */ override def acquireReadCompilationLock(): Long = { // CloseFileCmd does: diff --git a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/EnsureCompiledJob.scala b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/EnsureCompiledJob.scala index deb389b9031..950899762db 100644 --- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/EnsureCompiledJob.scala +++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/EnsureCompiledJob.scala @@ -378,7 +378,7 @@ final class EnsureCompiledJob( private def invalidateCaches( module: Module, changeset: Changeset[_] - )(implicit ctx: RuntimeContext, logger: TruffleLogger): Unit = { + )(implicit ctx: RuntimeContext): Unit = { val invalidationCommands = buildCacheInvalidationCommands( changeset, diff --git a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/UpsertVisualizationJob.scala b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/UpsertVisualizationJob.scala index a840399c1fa..d1fe27e0552 100644 --- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/UpsertVisualizationJob.scala +++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/UpsertVisualizationJob.scala @@ -62,6 +62,7 @@ class UpsertVisualizationJob( implicit val logger: TruffleLogger = ctx.executionService.getLogger val lockTimestamp = ctx.locking.acquireContextLock(config.executionContextId) + val writeLockTimestamp = ctx.locking.acquireWriteCompilationLock() try { val maybeCallable = UpsertVisualizationJob.evaluateVisualizationExpression( @@ -118,6 +119,11 @@ class UpsertVisualizationJob( } } } finally { + ctx.locking.releaseWriteCompilationLock() + logger.log( + Level.FINEST, + s"Kept write compilation lock [UpsertVisualizationJob] for ${System.currentTimeMillis() - writeLockTimestamp} milliseconds" + ) ctx.locking.releaseContextLock(config.executionContextId) logger.log( Level.FINEST, @@ -200,7 +206,7 @@ object UpsertVisualizationJob { */ def upsertVisualization( visualization: Visualization - )(implicit ctx: RuntimeContext, logger: TruffleLogger): Unit = + )(implicit ctx: RuntimeContext): Unit = visualization match { case visualization: Visualization.AttachedVisualization => val visualizationConfig = visualization.config @@ -289,6 +295,7 @@ object UpsertVisualizationJob { ): Either[EvaluationFailure, AnyRef] = { Either .catchNonFatal { + ctx.locking.assertWriteCompilationLock() ctx.executionService.evaluateExpression(module, argumentExpression) } .leftFlatMap { @@ -359,6 +366,7 @@ object UpsertVisualizationJob { .catchNonFatal { expression match { case Api.VisualizationExpression.Text(_, expression) => + ctx.locking.assertWriteCompilationLock() ctx.executionService.evaluateExpression( expressionModule, expression @@ -493,7 +501,7 @@ object UpsertVisualizationJob { visualizationConfig: Api.VisualizationConfiguration, callback: AnyRef, arguments: Vector[AnyRef] - )(implicit ctx: RuntimeContext, logger: TruffleLogger): Visualization = { + )(implicit ctx: RuntimeContext): Visualization = { val visualizationExpressionId = findVisualizationExpressionId(module, visualizationConfig.expression) val visualization = @@ -507,16 +515,7 @@ object UpsertVisualizationJob { callback, arguments ) - val writeLockTimestamp = ctx.locking.acquireWriteCompilationLock() - try { - invalidateCaches(visualization) - } finally { - ctx.locking.releaseWriteCompilationLock() - logger.log( - Level.FINEST, - s"Kept write compilation lock [UpsertVisualizationJob] for ${System.currentTimeMillis() - writeLockTimestamp} milliseconds" - ) - } + invalidateCaches(visualization) ctx.contextManager.upsertVisualization( visualizationConfig.executionContextId, visualization diff --git a/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbContext.java b/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbContext.java index 4e40d4e476b..79c97d70a76 100644 --- a/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbContext.java +++ b/engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbContext.java @@ -69,9 +69,9 @@ public class EpbContext { (Consumer) (context) -> { var lock = innerContext.enter(null); - log.log(Level.FINEST, "Entering initialization thread"); - cdl.countDown(); try { + log.log(Level.FINEST, "Entering initialization thread"); + cdl.countDown(); for (var l : langs.split(",")) { log.log(Level.FINEST, "Initializing language {0}", l); long then = System.currentTimeMillis();