Skip to content

Commit

Permalink
[DAP] Expand synchronized block to cover sending of stopped message
Browse files Browse the repository at this point in the history
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().
  • Loading branch information
agarciadom committed Sep 2, 2024
1 parent f5bcd85 commit 972a9a8
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -150,36 +153,36 @@ 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);
}

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;
}

Expand Down

0 comments on commit 972a9a8

Please sign in to comment.