Skip to content

Commit

Permalink
Don't send expression updates about interrupts (#11218)
Browse files Browse the repository at this point in the history
There is no value in sending expression updates involving interrupts to the user:
![Screenshot from 2024-09-30 14-47-17-2](https://github.com/user-attachments/assets/78fca5bf-085d-4c1c-99fb-0acb5f0a31a3)

Adding more logging information to see how aborts affect execution.

Related to #11084.
  • Loading branch information
hubertp authored Oct 1, 2024
1 parent 3a22147 commit ad53c82
Show file tree
Hide file tree
Showing 15 changed files with 231 additions and 54 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ public void executeSynchronously(RuntimeContext ctx, ExecutionContext ec) {
ctx.jobControlPlane()
.abortJobs(
contextId,
"execute expression for expression "
+ expressionId
+ " in visualization "
+ visualizationId,
job -> {
if (job instanceof ExecuteJob e) {
return e.visualizationTriggered();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ public Future<BoxedUnit> executeAsynchronously(RuntimeContext ctx, ExecutionCont
try {
logger.log(Level.FINE, "Invalidating modules, cancelling background jobs");
ctx.jobControlPlane().stopBackgroundJobs();
ctx.jobControlPlane().abortBackgroundJobs(DeserializeLibrarySuggestionsJob.class);
ctx.jobControlPlane()
.abortBackgroundJobs(
"invalidate modules index", DeserializeLibrarySuggestionsJob.class);

EnsoContext context = ctx.executionService().getContext();
context
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ private void setExecutionEnvironment(
var oldEnvironmentName =
ctx.executionService().getContext().getExecutionEnvironment().getName();
if (!oldEnvironmentName.equals(executionEnvironment.name())) {
ctx.jobControlPlane().abortJobs(contextId);
ctx.jobControlPlane()
.abortJobs(
contextId, "set execution environment to " + executionEnvironment.name());
ctx.locking()
.withWriteCompilationLock(
this.getClass(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@
public interface JobControlPlane {

/** Aborts all interruptible jobs. */
void abortAllJobs();
void abortAllJobs(String reason);

/**
* Abort all jobs except the ignored jobs.
*
* @param ignoredJobs the list of jobs to keep in the execution queue
*/
@SuppressWarnings("unchecked")
void abortAllExcept(Class<? extends Job<?>>... ignoredJobs);
void abortAllExcept(String reason, Class<? extends Job<?>>... ignoredJobs);

/**
* Aborts jobs that relates to the specified execution context.
Expand All @@ -25,7 +25,7 @@ public interface JobControlPlane {
* aborted
*/
@SuppressWarnings("unchecked")
void abortJobs(UUID contextId, Class<? extends Job<?>>... classOf);
void abortJobs(UUID contextId, String reason, Class<? extends Job<?>>... classOf);

/**
* Aborts jobs that relate to the specified execution context.
Expand All @@ -34,15 +34,16 @@ public interface JobControlPlane {
* @param accept filter that selects jobs to be aborted
*/
@SuppressWarnings("unchecked")
void abortJobs(UUID contextId, java.util.function.Function<Job<?>, Boolean> accept);
void abortJobs(
UUID contextId, String reason, java.util.function.Function<Job<?>, Boolean> accept);

/**
* Abort provided background jobs.
*
* @param toAbort the list of jobs to abort
*/
@SuppressWarnings("unchecked")
void abortBackgroundJobs(Class<? extends Job<?>>... toAbort);
void abortBackgroundJobs(String reason, Class<? extends Job<?>>... toAbort);

/**
* Starts background jobs processing.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.enso.interpreter.instrument.job;

import com.oracle.truffle.api.CompilerDirectives;
import com.oracle.truffle.api.interop.ExceptionType;
import com.oracle.truffle.api.interop.InteropLibrary;
import com.oracle.truffle.api.interop.UnsupportedMessageException;
import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -42,6 +43,28 @@ public static String findExceptionMessage(Throwable ex) {
}
}

public static boolean isInterruptedException(Throwable ex) {
var iop = InteropLibrary.getUncached();
return isInterruptedException(ex, iop);
}

private static boolean isInterruptedException(Object ex, InteropLibrary iop) {
try {
var interrupt = iop.getExceptionType(ex) == ExceptionType.INTERRUPT;
if (interrupt) {
return true;
}
try {
var cause = iop.getExceptionCause(ex);
return cause != null && isInterruptedException(cause, iop);
} catch (UnsupportedMessageException e) {
return false;
}
} catch (UnsupportedMessageException e) {
throw CompilerDirectives.shouldNotReachHere(e);
}
}

public static byte[] visualizationResultToBytes(Object value) {
if (value instanceof byte[] arr) {
return arr;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class DestroyContextCmd(
}

private def removeContext()(implicit ctx: RuntimeContext): Unit = {
ctx.jobControlPlane.abortJobs(request.contextId)
ctx.jobControlPlane.abortJobs(request.contextId, "destroy context")
val contextLock = ctx.locking.getOrCreateContextLock(request.contextId)
try {
ctx.locking.withContextLock(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class EditFileCmd(request: Api.EditFileNotification)
() => {
logger.log(
Level.FINEST,
"Adding pending file [{0}] edits [{1}] idMap [{2}]",
"Adding pending file [{0}] edits [{1}] and IdMap of length {2}",
Array[Any](
MaskedPath(request.path.toPath),
request.edits.map(e => (e.range, e.text.length)),
Expand All @@ -50,7 +50,7 @@ class EditFileCmd(request: Api.EditFileNotification)
ctx.state.pendingEdits.updateIdMap(request.path, idMap)
}
if (request.execute) {
ctx.jobControlPlane.abortAllJobs()
ctx.jobControlPlane.abortAllJobs("edit file")
ctx.jobProcessor
.run(compileJob())
.foreach(_ => executeJobs.foreach(ctx.jobProcessor.run))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class InterruptContextCmd(
): Future[Unit] =
if (doesContextExist) {
Future {
ctx.jobControlPlane.abortJobs(request.contextId)
ctx.jobControlPlane.abortJobs(request.contextId, "interrupt context")
reply(Api.InterruptContextResponse(request.contextId))
}
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class PopContextCmd(
ec: ExecutionContext
): Future[Unit] =
Future {
ctx.jobControlPlane.abortJobs(request.contextId)
ctx.jobControlPlane.abortJobs(request.contextId, "pop context")
val maybeTopItem = ctx.contextManager.pop(request.contextId)
if (maybeTopItem.isDefined) {
reply(Api.PopContextResponse(request.contextId))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class PushContextCmd(
ec: ExecutionContext
): Future[Boolean] =
Future {
ctx.jobControlPlane.abortJobs(request.contextId)
ctx.jobControlPlane.abortJobs(request.contextId, "push context")
val stack = ctx.contextManager.getStack(request.contextId)
val pushed = request.stackItem match {
case _: Api.StackItem.ExplicitCall if stack.isEmpty =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class RecomputeContextCmd(
ec: ExecutionContext
): Future[Boolean] = {
Future {
ctx.jobControlPlane.abortJobs(request.contextId)
ctx.jobControlPlane.abortJobs(request.contextId, "recompute context")
val stack = ctx.contextManager.getStack(request.contextId)
if (stack.isEmpty) {
reply(Api.EmptyStackError(request.contextId))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ class SetExpressionValueCmd(request: Api.SetExpressionValueNotification)
)
)
ctx.state.pendingEdits.enqueue(request.path, pendingApplyEdits)
ctx.jobControlPlane.abortAllJobs()
ctx.jobControlPlane.abortAllJobs(
"set expression value for expression " + request.expressionId
)
ctx.jobProcessor.run(new EnsureCompiledJob(Seq(request.path)))
executeJobs.foreach(ctx.jobProcessor.run)
Future.successful(())
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.enso.interpreter.instrument.execution

import com.oracle.truffle.api.TruffleLogger
import org.enso.interpreter.instrument.InterpreterContext
import org.enso.interpreter.instrument.job.{BackgroundJob, Job, UniqueJob}
import org.enso.text.Sha3_224VersionCalculator
Expand Down Expand Up @@ -74,6 +75,9 @@ final class JobExecutionEngine(
versionCalculator = Sha3_224VersionCalculator
)

private lazy val logger: TruffleLogger =
runtimeContext.executionService.getLogger

/** @inheritdoc */
override def runBackground[A](job: BackgroundJob[A]): Unit =
synchronized {
Expand Down Expand Up @@ -112,7 +116,7 @@ final class JobExecutionEngine(
allJobs.foreach { runningJob =>
runningJob.job match {
case jobRef: UniqueJob[_] if jobRef.equalsTo(job) =>
runtimeContext.executionService.getLogger
logger
.log(Level.FINEST, s"Cancelling duplicate job [$jobRef].")
runningJob.future.cancel(jobRef.mayInterruptIfRunning)
case _ =>
Expand All @@ -129,8 +133,11 @@ final class JobExecutionEngine(
): Future[A] = {
val jobId = UUID.randomUUID()
val promise = Promise[A]()
val logger = runtimeContext.executionService.getLogger
logger.log(Level.FINE, s"Submitting job: {0}...", job)
logger.log(
Level.FINE,
s"Submitting job: {0} with {1} id...",
Array(job, jobId)
)
val future = executorService.submit(() => {
logger.log(Level.FINE, s"Executing job: {0}...", job)
val before = System.currentTimeMillis()
Expand Down Expand Up @@ -166,17 +173,25 @@ final class JobExecutionEngine(
}

/** @inheritdoc */
override def abortAllJobs(): Unit =
abortAllExcept()
override def abortAllJobs(reason: String): Unit =
abortAllExcept(reason)

/** @inheritdoc */
override def abortAllExcept(ignoredJobs: Class[_ <: Job[_]]*): Unit = {
override def abortAllExcept(
reason: String,
ignoredJobs: Class[_ <: Job[_]]*
): Unit = {
val allJobs = runningJobsRef.updateAndGet(_.filterNot(_.future.isCancelled))
val cancellableJobs = allJobs
.filter { runningJob =>
runningJob.job.isCancellable &&
!ignoredJobs.contains(runningJob.job.getClass)
}
logger.log(
Level.FINE,
"Aborting {0} jobs because {1}: {2}",
Array(cancellableJobs.length, reason, cancellableJobs.map(_.id))
)
cancellableJobs.foreach { runningJob =>
runningJob.future.cancel(runningJob.job.mayInterruptIfRunning)
}
Expand All @@ -187,6 +202,7 @@ final class JobExecutionEngine(
/** @inheritdoc */
override def abortJobs(
contextId: UUID,
reason: String,
toAbort: Class[_ <: Job[_]]*
): Unit = {
val allJobs = runningJobsRef.get()
Expand All @@ -196,6 +212,11 @@ final class JobExecutionEngine(
runningJob.job.isCancellable && (toAbort.isEmpty || toAbort
.contains(runningJob.getClass))
) {
logger.log(
Level.FINE,
"Aborting job {0} because {1}",
Array(runningJob.id, reason)
)
runningJob.future.cancel(runningJob.job.mayInterruptIfRunning)
}
}
Expand All @@ -206,27 +227,41 @@ final class JobExecutionEngine(
/** @inheritdoc */
override def abortJobs(
contextId: UUID,
reason: String,
accept: java.util.function.Function[Job[_], java.lang.Boolean]
): Unit = {
val allJobs = runningJobsRef.get()
val contextJobs = allJobs.filter(_.job.contextIds.contains(contextId))
contextJobs.foreach { runningJob =>
if (runningJob.job.isCancellable && accept.apply(runningJob.job)) {
logger.log(
Level.FINE,
"Aborting job {0} because {1}",
Array(runningJob.id, reason)
)
runningJob.future.cancel(runningJob.job.mayInterruptIfRunning)
}
}
runtimeContext.executionService.getContext.getThreadManager
.interruptThreads()
}

override def abortBackgroundJobs(toAbort: Class[_ <: Job[_]]*): Unit = {
override def abortBackgroundJobs(
reason: String,
toAbort: Class[_ <: Job[_]]*
): Unit = {
val allJobs =
backgroundJobsRef.updateAndGet(_.filterNot(_.future.isCancelled))
val cancellableJobs = allJobs
.filter { runningJob =>
runningJob.job.isCancellable &&
toAbort.contains(runningJob.job.getClass)
}
logger.log(
Level.FINE,
"Aborting {0} background jobs because {1}: {2}",
Array(cancellableJobs.length, reason, cancellableJobs.map(_.id))
)
cancellableJobs.foreach { runningJob =>
runningJob.future.cancel(runningJob.job.mayInterruptIfRunning)
}
Expand Down
Loading

0 comments on commit ad53c82

Please sign in to comment.