Skip to content

Commit

Permalink
chore: fix flaky Windows CI
Browse files Browse the repository at this point in the history
  • Loading branch information
@jansorg @ahtrotta
jansorg authored and ahtrotta committed Mar 1, 2024
1 parent 0f1a05e commit ab556b1
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 48 deletions.
9 changes: 9 additions & 0 deletions plugin-core/src/main/java/appland/cli/AppLandCommandLineService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import java.nio.file.Path;
import java.util.List;
import java.util.concurrent.TimeUnit;

public interface AppLandCommandLineService extends Disposable {
static @NotNull AppLandCommandLineService getInstance() {
Expand Down Expand Up @@ -76,6 +77,14 @@ public interface AppLandCommandLineService extends Disposable {
*/
void stopAll(boolean waitForTermination);

/**
* Stop all processes.
*
* @param timeout Length of timeout to wait for process termination. 0 disables the waiting for termination.
* @param timeUnit Unit of timeout.
*/
void stopAll(int timeout, @NotNull TimeUnit timeUnit);

/**
* Creates the command line to install AppMap in the given directory.
*
Expand Down
31 changes: 19 additions & 12 deletions plugin-core/src/main/java/appland/cli/DefaultCommandLineService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -108,7 +109,7 @@ public synchronized void start(@NotNull VirtualFile directory, boolean waitForPr
public synchronized void stop(@NotNull VirtualFile directory, boolean waitForTermination) {
var entry = processes.remove(directory);
if (entry != null) {
stopLocked(entry, waitForTermination);
stopLocked(entry, waitForTermination ? 1_000 : 0, TimeUnit.MILLISECONDS);
}
}

Expand Down Expand Up @@ -153,6 +154,11 @@ public void restartProcessesInBackground() {

@Override
public void stopAll(boolean waitForTermination) {
stopAll(waitForTermination ? 1_000 : 0, TimeUnit.MILLISECONDS);
}

@Override
public void stopAll(int timeout, @NotNull TimeUnit timeUnit) {
List<CliProcesses> activeProcesses;
synchronized (this) {
activeProcesses = List.copyOf(processes.values());
Expand All @@ -161,7 +167,7 @@ public void stopAll(boolean waitForTermination) {

for (var processes : activeProcesses) {
try {
stopLocked(processes, waitForTermination);
stopLocked(processes, timeout, timeUnit);
} catch (Exception e) {
LOG.warn("Error shutting down processes on disposal", e);
}
Expand Down Expand Up @@ -219,12 +225,13 @@ public void stopAll(boolean waitForTermination) {
/**
* Stops processes, but does not modify {@link #processes}. It's the responsibility of the caller to update it.
*
* @param processes Processes to stop
* @param waitForTermination If this method should wait until the processes are terminated
* @param processes Processes to stop
* @param timeout Timeout to wait for process termination.
* @param timeUnit Unit of timeout.
*/
private void stopLocked(@NotNull CliProcesses processes, boolean waitForTermination) {
stopProcess(processes.indexer, waitForTermination);
stopProcess(processes.scanner, waitForTermination);
private void stopLocked(@NotNull CliProcesses processes, int timeout, @NotNull TimeUnit timeUnit) {
stopProcess(processes.indexer, timeout, timeUnit);
stopProcess(processes.scanner, timeout, timeUnit);
}

@Override
Expand Down Expand Up @@ -264,7 +271,7 @@ protected void requestVirtualFileRefresh(@NotNull Path path) {
} catch (ExecutionException e) {
LOG.debug("Error starting scanner process", e);
if (indexer != null) {
stopProcess(indexer, false);
stopProcess(indexer, 0, TimeUnit.MILLISECONDS);
}
return null;
}
Expand Down Expand Up @@ -436,7 +443,7 @@ private static boolean isSupported() {
};
}

private static void stopProcess(@Nullable KillableProcessHandler process, boolean waitForTermination) {
private static void stopProcess(@Nullable KillableProcessHandler process, int timeout, @NotNull TimeUnit timeUnit) {
if (process == null) {
return;
}
Expand All @@ -453,13 +460,13 @@ public void run() {
process.killProcess();
}

if (waitForTermination) {
process.waitFor(1_000);
if (timeout > 0) {
process.waitFor(timeUnit.toMillis(timeout));
}
}
};

if (waitForTermination) {
if (timeout > 0) {
ApplicationManager.getApplication().executeOnPooledThread(shutdownRunnable).get();
} else {
shutdownRunnable.run();
Expand Down
73 changes: 38 additions & 35 deletions plugin-core/src/test/java/appland/AppMapBaseTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,60 +20,47 @@
import com.intellij.util.ThrowableRunnable;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;

import java.util.Collections;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;

public abstract class AppMapBaseTest extends LightPlatformCodeInsightFixture4TestCase {
@Before
public void cleanupTempFilesystem() {
if (ApplicationManager.getApplication().isDispatchThread()) {
TempFileSystem.getInstance().cleanupForNextTest();
} else {
edt(() -> TempFileSystem.getInstance().cleanupForNextTest());
}
}

@Override
protected LightProjectDescriptor getProjectDescriptor() {
// we're returning a new instance, because we don't want to share the project setup between light tests.
// many of our tests require a clean filesystem.
return new LightProjectDescriptor();
}

@After
public void resetState() {
TestCommandLineService.getInstance().reset();
@Override
protected void setUp() throws Exception {
super.setUp();

if (ApplicationManager.getApplication().isDispatchThread()) {
TempFileSystem.getInstance().cleanupForNextTest();
} else {
edt(() -> TempFileSystem.getInstance().cleanupForNextTest());
}
}

@After
public void shutdownAppMapProcesses() {
var commandLineService = AppLandCommandLineService.getInstance();
@Override
protected void tearDown() throws Exception {
try {
// multiple shutdown attempts because Windows CI is constantly failing
var attempts = 5;
for (var i = 1; i <= attempts; i++) {
if (commandLineService.getActiveRoots().isEmpty()) {
break;
}
try {
resetState();
} catch (Throwable e) {
addSuppressedException(e);
}

LOG.debug(String.format("Attempting to terminate AppMap processes: %d/%d", i, attempts));
if (i > 1) {
Thread.sleep(5_000);
}
commandLineService.stopAll(true);
try {
shutdownAppMapProcesses();
} catch (Throwable e) {
addSuppressedException(e);
}
} catch (Throwable t) {
addSuppressedException(t);
} finally {
// reset to default settings
ApplicationManager.getApplication().getService(AppMapApplicationSettingsService.class).loadState(new AppMapApplicationSettings());

var activeRoots = commandLineService.getActiveRoots();
Assert.assertTrue("All AppMap CLIs must be terminated: " + activeRoots, activeRoots.isEmpty());
super.tearDown();
}
}

Expand Down Expand Up @@ -223,4 +210,20 @@ private String createClassMap(int functionCount) {
json.append("]");
return json.toString();
}

private void resetState() {
TestCommandLineService.getInstance().reset();
}

private void shutdownAppMapProcesses() {
try {
AppLandCommandLineService.getInstance().stopAll(10_000, TimeUnit.MILLISECONDS);
} finally {
// reset to default settings
ApplicationManager.getApplication().getService(AppMapApplicationSettingsService.class).loadState(new AppMapApplicationSettings());

var activeRoots = AppLandCommandLineService.getInstance().getActiveRoots();
Assert.assertTrue("All AppMap CLIs must be terminated: " + activeRoots, activeRoots.isEmpty());
}
}
}
4 changes: 3 additions & 1 deletion plugin-java/src/test/java/appland/execution/BaseAppMapJavaTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import com.intellij.testFramework.EdtTestUtil;
import com.intellij.testFramework.JavaPsiTestCase;

import java.util.concurrent.TimeUnit;

public abstract class BaseAppMapJavaTest extends JavaPsiTestCase {
@Override
protected void setUp() throws Exception {
Expand All @@ -29,7 +31,7 @@ protected void setUp() throws Exception {
@Override
protected void tearDown() throws Exception {
try {
AppLandCommandLineService.getInstance().stopAll(true);
AppLandCommandLineService.getInstance().stopAll(10_000, TimeUnit.MILLISECONDS);
} catch (Exception e) {
addSuppressedException(e);
} finally {
Expand Down

0 comments on commit ab556b1

Please sign in to comment.