Skip to content

Commit

Permalink
Eliminate deadlock scenario during file edit/open/close requests
Browse files Browse the repository at this point in the history
At the beginning of the execution `EnsureCompiledJob` acquired write
compilation lock. When compiling individual modules it would then
- acquire file lock
- acquire read compilation lock

The second one was spurious since it already kept the write lock.
This sequence meant however that `CloseFileCmd` or `OpenFileCmd` can
lead to  a deadlock when requests come in close succession. This is
because commands:
- acquire file lock
- acquire read compilation lock

So `EnsureCompiledJob` might have the (write) compilation lock but the
commands could have file lock. And the second required lock for either
the job or the command could never be acquired.

Flipping the order did the trick.

Partially solves #6841.
  • Loading branch information
hubertp committed Jun 1, 2023
1 parent c6cb937 commit 5acb56c
Show file tree
Hide file tree
Showing 5 changed files with 7 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -345,10 +345,7 @@ public void setModuleSources(File path, String contents) {
if (module.isEmpty()) {
module = context.createModuleForFile(path);
}
module.ifPresent(
mod -> {
mod.setLiteralSource(contents);
});
module.ifPresent(mod -> mod.setLiteralSource(contents));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ class CloseFileCmd(request: Api.CloseFileNotification) extends Command(None) {
ec: ExecutionContext
): Future[Unit] =
Future {
ctx.locking.acquireFileLock(request.path)
ctx.locking.acquireReadCompilationLock()
ctx.locking.acquireFileLock(request.path)
try {
ctx.executionService.resetModuleSources(request.path)
} finally {
ctx.locking.releaseReadCompilationLock()
ctx.locking.releaseFileLock(request.path)
ctx.locking.releaseReadCompilationLock()
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,16 @@ class OpenFileCmd(request: Api.OpenFileNotification) extends Command(None) {
ec: ExecutionContext
): Future[Unit] =
Future {
ctx.locking.acquireFileLock(request.path)
ctx.locking.acquireReadCompilationLock()
ctx.locking.acquireFileLock(request.path)
try {
ctx.executionService.setModuleSources(
request.path,
request.contents
)
} finally {
ctx.locking.releaseReadCompilationLock()
ctx.locking.releaseFileLock(request.path)
ctx.locking.releaseReadCompilationLock()
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,6 @@ final class EnsureCompiledJob(protected val files: Iterable[File])
file: File
)(implicit ctx: RuntimeContext): Option[Changeset[Rope]] = {
ctx.locking.acquireFileLock(file)
ctx.locking.acquireReadCompilationLock()
ctx.locking.acquirePendingEditsLock()
try {
val pendingEdits = ctx.state.pendingEdits.dequeue(file)
Expand All @@ -253,7 +252,6 @@ final class EnsureCompiledJob(protected val files: Iterable[File])
Option.when(shouldExecute)(changeset)
} finally {
ctx.locking.releasePendingEditsLock()
ctx.locking.releaseReadCompilationLock()
ctx.locking.releaseFileLock(file)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ class ExecuteJob(

/** @inheritdoc */
override def run(implicit ctx: RuntimeContext): Unit = {
ctx.locking.acquireContextLock(contextId)
ctx.locking.acquireReadCompilationLock()
ctx.locking.acquireContextLock(contextId)
val context = ctx.executionService.getContext
val originalExecutionEnvironment =
executionEnvironment.map(_ => context.getExecutionEnvironment)
Expand All @@ -48,8 +48,8 @@ class ExecuteJob(
}
} finally {
originalExecutionEnvironment.foreach(context.setExecutionEnvironment)
ctx.locking.releaseReadCompilationLock()
ctx.locking.releaseContextLock(contextId)
ctx.locking.releaseReadCompilationLock()
}
ctx.endpoint.sendToClient(Api.Response(Api.ExecutionComplete(contextId)))
StartBackgroundProcessingJob.startBackgroundJobs()
Expand Down

0 comments on commit 5acb56c

Please sign in to comment.