From 04c95465736fa60931ed65bbe79ecc6b26fc8bf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Wolski?= Date: Wed, 9 Jan 2019 10:02:31 +0100 Subject: [PATCH 1/2] Engine: fix assertion error caused by emError The main loop assumes that the engine is busy, however the handling of the emError state sets engineBusy to false. This causes an assertion, that was added in c750929c via #26, to trigger, which leads in debug runs (-ea flag set) to a crashed Engine and a blocked TestEngineDriver. Additionaly the engineBusy flag is set to false even when the Engine is actually busy as the emError state is left to either emIdle or emTerminating. This commit 1) changes the assertion to not throw when in emError state and 2) corrects the setting of engineBusy to be false only when no ecTerminate / ecRecover command was handled. TODO: This basic fix leaves the Engine in a Thread.yield loop until a ecTerminate / ecRecover command is present. The polling of the commandQueue should be replaced with a blocking wait. --- .../src/org/coreasm/engine/Engine.java | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/org.coreasm.engine/src/org/coreasm/engine/Engine.java b/org.coreasm.engine/src/org/coreasm/engine/Engine.java index 3b517e05..87a9f4d9 100644 --- a/org.coreasm.engine/src/org/coreasm/engine/Engine.java +++ b/org.coreasm.engine/src/org/coreasm/engine/Engine.java @@ -801,11 +801,11 @@ public EngineThread(String name) { public void run() { try { while (!terminating) { - assert engineBusy; - try { EngineMode engineMode = getEngineMode(); + assert engineBusy || (engineMode == EngineMode.emError); + // if an error is occurred and the engine is not // in error mode, go to the error mode if (lastError != null @@ -987,6 +987,8 @@ public void run() { */ case emError: + boolean hasNextCommand = false; + isBusyLock.lock(); try { // Throw out all commands except a recovery command @@ -994,24 +996,36 @@ public void run() { while ((cmd = commandQueue.poll()) != null) { if (cmd.type == EngineCommand.CmdType.ecTerminate) { next(EngineMode.emTerminating); + hasNextCommand = true; lastError = null; logger.debug("Engine terminated by user command."); break; } else if (cmd.type == EngineCommand.CmdType.ecRecover) { // Recover by going to the idle mode next(EngineMode.emIdle); + hasNextCommand = true; lastError = null; logger.debug("Engine recovered from error by user command."); break; } } - engineBusy = false; - isNotBusy.signalAll(); + if (!hasNextCommand) { + engineBusy = false; + isNotBusy.signalAll(); + } + else { + assert engineBusy; + } } finally { isBusyLock.unlock(); } + + if (!hasNextCommand) { + Thread.yield(); + } + break; case emTerminated: break; default: From 1089199123ae5fc05a0576d57c89efd0826d6752 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Wolski?= Date: Wed, 9 Jan 2019 15:41:04 +0100 Subject: [PATCH 2/2] Engine: replace yield in emError handling.. .. with blocking wait for the next command. Also restore the assertion that was changed in the previous commit as workaround. --- .../src/org/coreasm/engine/Engine.java | 64 +++++++++---------- 1 file changed, 30 insertions(+), 34 deletions(-) diff --git a/org.coreasm.engine/src/org/coreasm/engine/Engine.java b/org.coreasm.engine/src/org/coreasm/engine/Engine.java index 87a9f4d9..60b31f8f 100644 --- a/org.coreasm.engine/src/org/coreasm/engine/Engine.java +++ b/org.coreasm.engine/src/org/coreasm/engine/Engine.java @@ -801,11 +801,11 @@ public EngineThread(String name) { public void run() { try { while (!terminating) { + assert engineBusy; + try { EngineMode engineMode = getEngineMode(); - assert engineBusy || (engineMode == EngineMode.emError); - // if an error is occurred and the engine is not // in error mode, go to the error mode if (lastError != null @@ -987,44 +987,40 @@ public void run() { */ case emError: - boolean hasNextCommand = false; - - isBusyLock.lock(); - try { - // Throw out all commands except a recovery command - EngineCommand cmd; - while ((cmd = commandQueue.poll()) != null) { - if (cmd.type == EngineCommand.CmdType.ecTerminate) { - next(EngineMode.emTerminating); - hasNextCommand = true; - lastError = null; - logger.debug("Engine terminated by user command."); - break; - } else if (cmd.type == EngineCommand.CmdType.ecRecover) { - // Recover by going to the idle mode - next(EngineMode.emIdle); - hasNextCommand = true; - lastError = null; - logger.debug("Engine recovered from error by user command."); - break; + // blocking wait for termination or recover command + // we are not busy while waiting + // throw out all other commands + while (true) { + isBusyLock.lock(); + try { + if (commandQueue.isEmpty()) { + engineBusy = false; + isNotBusy.signalAll(); } } - - if (!hasNextCommand) { - engineBusy = false; - isNotBusy.signalAll(); + finally { + isBusyLock.unlock(); } - else { - assert engineBusy; + + EngineCommand cmd = commandQueue.take(); + assert engineBusy; + + if (cmd.type == EngineCommand.CmdType.ecTerminate) { + next(EngineMode.emTerminating); + lastError = null; + logger.debug("Engine terminated by user command."); + break; + } else if (cmd.type == EngineCommand.CmdType.ecRecover) { + // Recover by going to the idle mode + next(EngineMode.emIdle); + lastError = null; + logger.debug("Engine recovered from error by user command."); + break; + } else { + logger.warn("Ignore user command {}, Engine is in error state and needs to be terminated or reset", cmd.type); } } - finally { - isBusyLock.unlock(); - } - if (!hasNextCommand) { - Thread.yield(); - } break; case emTerminated: break;