From 972a9a8c868daf896542d23c20fa4fc188ef8ce4 Mon Sep 17 00:00:00 2001 From: Antonio Garcia-Dominguez Date: Mon, 2 Sep 2024 12:57:06 +0100 Subject: [PATCH] [DAP] Expand synchronized block to cover sending of stopped message When running tests from Github Actions, there was a race condition between the code handling the "stopped" message and the code that was trying to continue the execution after the end of the test. Sometimes, the test would try to release the "suspendLatch" before it was set, which would result in the "continue" request effectively being ignored by the debug adapter. This commit extends the synchronized block in the suspend() method so any code trying to call resumeAllThreads() while reacting to the "stopped" message will have to wait until the suspendLatch has been enabled and the execution of the Epsilon script has been suspended by having the Thread wait(). --- .../epsilon/eol/dap/EpsilonDebugAdapter.java | 27 +++++++++++-------- .../test/AbstractEpsilonDebugAdapterTest.java | 23 +++++++++------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/plugins/org.eclipse.epsilon.eol.dap/src/org/eclipse/epsilon/eol/dap/EpsilonDebugAdapter.java b/plugins/org.eclipse.epsilon.eol.dap/src/org/eclipse/epsilon/eol/dap/EpsilonDebugAdapter.java index a477818b5..905843f3b 100644 --- a/plugins/org.eclipse.epsilon.eol.dap/src/org/eclipse/epsilon/eol/dap/EpsilonDebugAdapter.java +++ b/plugins/org.eclipse.epsilon.eol.dap/src/org/eclipse/epsilon/eol/dap/EpsilonDebugAdapter.java @@ -910,18 +910,23 @@ private String getStackFrameName(int position, Frame frame) { } protected void suspend(int threadId, ModuleElement ast, SuspendReason reason) throws InterruptedException { - switch (reason) { - case STEP: - sendStopped(threadId, StoppedEventArgumentsReason.STEP); - break; - case BREAKPOINT: - sendStopped(threadId, StoppedEventArgumentsReason.BREAKPOINT); - break; - default: - throw new IllegalArgumentException("Unknown suspend reason"); - } - synchronized (suspendedLatch) { + /* + * Note: the synchronized region must cover the sending of stopped messages, + * as otherwise there is the risk that someone reacting to the "stopped" message + * may try to release the suspendedLatch before it has actually been set. + */ + switch (reason) { + case STEP: + sendStopped(threadId, StoppedEventArgumentsReason.STEP); + break; + case BREAKPOINT: + sendStopped(threadId, StoppedEventArgumentsReason.BREAKPOINT); + break; + default: + throw new IllegalArgumentException("Unknown suspend reason"); + } + suspendedLatch.set(true); do { final int timeoutMillis = 500; diff --git a/tests/org.eclipse.epsilon.eol.dap.test/src/org/eclipse/epsilon/eol/dap/test/AbstractEpsilonDebugAdapterTest.java b/tests/org.eclipse.epsilon.eol.dap.test/src/org/eclipse/epsilon/eol/dap/test/AbstractEpsilonDebugAdapterTest.java index f1bad9371..e7e3bfe33 100644 --- a/tests/org.eclipse.epsilon.eol.dap.test/src/org/eclipse/epsilon/eol/dap/test/AbstractEpsilonDebugAdapterTest.java +++ b/tests/org.eclipse.epsilon.eol.dap.test/src/org/eclipse/epsilon/eol/dap/test/AbstractEpsilonDebugAdapterTest.java @@ -56,8 +56,11 @@ public abstract class AbstractEpsilonDebugAdapterTest { + /** Timeout used for various assertions in this base class. */ + private static final int TIMEOUT_SECONDS = 5; + @Rule - public Timeout globalTimeout = Timeout.seconds(10); + public Timeout globalTimeout = Timeout.seconds(TIMEOUT_SECONDS * 2); protected class TestClient implements IDebugProtocolClient { private Semaphore isStopped = new Semaphore(0); @@ -150,28 +153,28 @@ public void teardown() { } protected void assertStoppedBecauseOf(final String expectedReason) throws InterruptedException { - boolean acquired = client.isStopped.tryAcquire(5, TimeUnit.SECONDS); - assertTrue("The script should have stopped within 5s", acquired); + boolean acquired = client.isStopped.tryAcquire(TIMEOUT_SECONDS, TimeUnit.SECONDS); + assertTrue("The script should have stopped within " + TIMEOUT_SECONDS + " seconds", acquired); assertNotNull("The script should have provided stopping arguments", client.stoppedArgs); assertEquals("The script should stop for the expected reason" , expectedReason, client.stoppedArgs.getReason()); } protected void assertProgramCompletedSuccessfully() throws InterruptedException { - boolean acquired = client.isExited.tryAcquire(5, TimeUnit.SECONDS); - assertTrue("The script should have exited within 5s", acquired); + boolean acquired = client.isExited.tryAcquire(TIMEOUT_SECONDS, TimeUnit.SECONDS); + assertTrue("The script should have exited within " + TIMEOUT_SECONDS + " seconds", acquired); assertNotNull("The script should have provided exiting arguments", client.exitedArgs); assertEquals("The script should have completed its execution successfully", 0, client.exitedArgs.getExitCode()); } protected void assertProgramFailed() throws InterruptedException { - client.isExited.tryAcquire(5, TimeUnit.SECONDS); - assertNotNull("The script should have exited within 5s", client.exitedArgs); + client.isExited.tryAcquire(TIMEOUT_SECONDS, TimeUnit.SECONDS); + assertNotNull("The script should have exited within " + TIMEOUT_SECONDS + " seconds", client.exitedArgs); assertEquals("The script should have completed its execution with an error", 1, client.exitedArgs.getExitCode()); } protected StackTraceResponse getStackTrace() throws Exception { - ThreadsResponse threads = adapter.threads().get(5, TimeUnit.SECONDS); - assertEquals("The debugger should report one thread", 1, threads.getThreads().length); + ThreadsResponse threads = adapter.threads().get(TIMEOUT_SECONDS, TimeUnit.SECONDS); + assertEquals("The debugger should report one thread within " + TIMEOUT_SECONDS + " seconds", 1, threads.getThreads().length); final int threadId = threads.getThreads()[0].getId(); return getStackTrace(threadId); @@ -179,7 +182,7 @@ protected StackTraceResponse getStackTrace() throws Exception { protected StackTraceResponse getStackTrace(final int threadId) throws Exception { final StackTraceArguments stackTraceArgs = createStackTraceArgs(threadId); - StackTraceResponse stackTrace = adapter.stackTrace(stackTraceArgs).get(5, TimeUnit.SECONDS); + StackTraceResponse stackTrace = adapter.stackTrace(stackTraceArgs).get(TIMEOUT_SECONDS, TimeUnit.SECONDS); return stackTrace; }