Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define and enforce locking order during runtime #8014

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,19 @@ abstract class SynchronousCommand(maybeRequestId: Option[RequestId])
} catch {
case _: InterruptedException | _: ThreadInterruptedException =>
Interrupted
case ex: Throwable =>
logger.log(
Level.SEVERE,
s"An error occurred during execution of $this command",
ex
)
case ex: Throwable => {
val msg = s"An error occurred during execution of $this command"
try {
logger.log(Level.SEVERE, msg, ex)
} catch {
case ise: IllegalStateException =>
// Thread using TruffleLogger has to have a current context or the TruffleLogger has to be bound to an engine
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To simulate the problem:

enso$ git diff
diff --git engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/ReentrantLocking.scala engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/ReentrantLocking.scala
index 8cb4b0b5c4..0b84019fd0 100644
--- engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/ReentrantLocking.scala
+++ engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/ReentrantLocking.scala
@@ -153,6 +153,8 @@ class ReentrantLocking(logger: TruffleLogger) extends Locking {
 
   /** @inheritdoc */
   override def acquireFileLock(file: File): Long = {
+    assertNotLocked(compilationLock, false, "wrong")
+    assertNotLocked(compilationLock, true, "wrong")
     // cannot have pendings lock as of EnsureCompiledJob.applyEdits
     assertNotLocked(
       pendingEditsLock,
$ sbt  runtime-with-instruments/test

and one gets:

java.lang.IllegalStateException: Thread using TruffleLogger has to have a current context or the TruffleLogger has to be bound to an engine., took 0.025 sec
[error]     at com.oracle.truffle.api.TruffleLogger$LoggerCache.noContext(TruffleLogger.java:1011)
[error]     at com.oracle.truffle.api.TruffleLogger$LoggerCache.isLoggable(TruffleLogger.java:993)
[error]     at com.oracle.truffle.api.TruffleLogger.isLoggableSlowPath(TruffleLogger.java:727)
[error]     at com.oracle.truffle.api.TruffleLogger.isLoggable(TruffleLogger.java:722)
[error]     at com.oracle.truffle.api.TruffleLogger.log(TruffleLogger.java:544)
[error]     at org.enso.interpreter.instrument.command.SynchronousCommand.execute(SynchronousCommand.scala:30)
[error]     at org.enso.interpreter.instrument.command.SynchronousCommand.execute(SynchronousCommand.scala:11)
[error]     at org.enso.interpreter.instrument.execution.CommandExecutionEngine.invoke(CommandExecutionEngine.scala:63)
[error]     at org.enso.interpreter.instrument.Handler.onMessage(Handler.scala:128)
[error]     at org.enso.interpreter.instrument.Endpoint.$anonfun$sendBinary$1(Handler.scala:66)
[error]     at org.enso.interpreter.instrument.Endpoint.$anonfun$sendBinary$1$adapted(Handler.scala:64)
[error]     at scala.Option.foreach(Option.scala:437)
[error]     at org.enso.interpreter.instrument.Endpoint.sendBinary(Handler.scala:64)
[error]     at com.oracle.truffle.api.instrumentation.TruffleInstrument$Env$MessageTransportProxy$MessageEndpointProxy.sendBinary(TruffleInstrument.java:1090)
[error]     at org.enso.interpreter.test.instrument.RuntimeServerEmulator.sendToRuntime(RuntimeServerEmulator.scala:53)
[error]     at org.enso.interpreter.test.instrument.RuntimeServerTest$TestContext.send(RuntimeServerTest.scala:94)
[error]     at org.enso.interpreter.test.instrument.IncrementalUpdatesTest.sendUpdatesWhenFunctionBodyIsChanged(IncrementalUpdatesTest.java:202)
[error]     at org.enso.interpreter.test.instrument.IncrementalUpdatesTest.sendUpdatesWhenFunctionBodyIsChangedBySettingValue(IncrementalUpdatesTest.java:150)
[error]     at org.enso.interpreter.test.instrument.IncrementalUpdatesTest.sendUpdatesWhenTextIsChangedBySettingValue(IncrementalUpdatesTest.java:105)

maybe we should be using normal logger and not Truffle logger here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This remains an open question, @hubertp: Shall we use regular, non-truffle logger here?

Copy link
Collaborator

@hubertp hubertp Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, very much so #8147 (better to file a separate ticket or the problem will get lost).

ex.printStackTrace()
ise.initCause(ex)
throw ise
}
Done
}
} finally {
logger.log(Level.FINE, s"Command $this finished.")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,25 @@ import java.util.logging.Level
*/
class ReentrantLocking(logger: TruffleLogger) extends Locking {

private val compilationLock = new ReentrantReadWriteLock(true)

/** Lowest level lock obtainable at any time. */
private val pendingEditsLock = new ReentrantLock()

private val contextMapLock = new ReentrantLock()
/** Obtain anytime, except when holding pendingsEditLock. Guarded by fileMapLock. */
private var fileLocks = Map.empty[File, ReentrantLock]

/** Lower than contextLocks. Higher than anything else. */
private val compilationLock = new ReentrantReadWriteLock(true)

/** The highest lock. Always obtain first. Guarded by contextMapLock. */
private var contextLocks = Map.empty[UUID, ReentrantLock]

private val fileMapLock = new ReentrantLock()
/** Guards contextLocks */
private val contextMapLock = new ReentrantLock()

private var fileLocks = Map.empty[File, ReentrantLock]
/** Guards fileLocks */
private val fileMapLock = new ReentrantLock()

protected def getContextLock(contextId: UUID): Lock = {
private def getContextLock(contextId: UUID): Lock = {
contextMapLock.lock()
try {
if (contextLocks.contains(contextId)) {
Expand All @@ -49,7 +55,7 @@ class ReentrantLocking(logger: TruffleLogger) extends Locking {
}
}

protected def getFileLock(file: File): Lock = {
private def getFileLock(file: File): Lock = {
fileMapLock.lock()
try {
if (fileLocks.contains(file)) {
Expand All @@ -76,6 +82,16 @@ class ReentrantLocking(logger: TruffleLogger) extends Locking {

/** @inheritdoc */
override def acquireWriteCompilationLock(): Long = {
assertNotLocked(
compilationLock,
true,
"Cannot upgrade compilation read lock to write lock"
)
assertNotLocked(
pendingEditsLock,
s"Cannot acquire compilation write lock when having pending edits lock"
)
assertNoFileLock("Cannot acquire write compilation lock")
logLockAcquisition(compilationLock.writeLock(), "write compilation")
}

Expand All @@ -85,6 +101,16 @@ class ReentrantLocking(logger: TruffleLogger) extends Locking {

/** @inheritdoc */
override def acquireReadCompilationLock(): Long = {
// CloseFileCmd does:
// ctx.locking.acquireReadCompilationLock()
// ctx.locking.acquireFileLock(request.path)
assertNoFileLock("Cannot acquire read compilation lock")
// CloseFileCmd also adds:
// ctx.locking.acquirePendingEditsLock()
assertNotLocked(
pendingEditsLock,
s"Cannot acquire compilation read lock when having pending edits lock"
)
logLockAcquisition(compilationLock.readLock(), "read compilation")
}

Expand All @@ -103,6 +129,21 @@ class ReentrantLocking(logger: TruffleLogger) extends Locking {

/** @inheritdoc */
override def acquireContextLock(contextId: UUID): Long = {
assertNotLocked(
compilationLock,
true,
s"Cannot acquire context ${contextId} lock when having compilation read lock"
)
assertNotLocked(
compilationLock,
false,
s"Cannot acquire context ${contextId} lock when having compilation write lock"
)
assertNoFileLock(s"Cannot acquire context ${contextId}")
assertNotLocked(
pendingEditsLock,
s"Cannot acquire context ${contextId} lock when having pending edits lock"
)
logLockAcquisition(getContextLock(contextId), s"$contextId context")
}

Expand All @@ -112,6 +153,12 @@ class ReentrantLocking(logger: TruffleLogger) extends Locking {

/** @inheritdoc */
override def acquireFileLock(file: File): Long = {
// cannot have pendings lock as of EnsureCompiledJob.applyEdits
assertNotLocked(
pendingEditsLock,
s"Cannot acquire file ${file} lock when having pending edits lock"
)
assertNoContextLock(s"Cannot acquire file ${file}")
logLockAcquisition(getFileLock(file), "file")
}

Expand All @@ -126,4 +173,46 @@ class ReentrantLocking(logger: TruffleLogger) extends Locking {
now2
}

private def assertNotLocked(
lock: ReentrantReadWriteLock,
read: Boolean,
msg: String
) = {
val locked =
if (read) lock.getReadHoldCount() > 0
else lock.isWriteLockedByCurrentThread()
if (locked) {
throw new IllegalStateException(msg)
}
}
private def assertNotLocked(lock: ReentrantLock, msg: String) = {
if (lock.isHeldByCurrentThread()) {
throw new IllegalStateException(msg)
}
}

private def assertNoFileLock(msg: String) = {
fileMapLock.lock()
try {
for (ctx <- fileLocks) {
assertNotLocked(ctx._2, msg + s" when having file ${ctx._1} lock")
}
} finally {
fileMapLock.unlock()
}
}

private def assertNoContextLock(msg: String) = {
contextMapLock.lock()
try {
for (ctx <- contextLocks) {
assertNotLocked(
ctx._2,
msg + s" lock when having context ${ctx._1} lock"
)
}
} finally {
contextMapLock.unlock()
}
}
}