From cc4950ea9fff9ca637659e9071a278d03a25b80e Mon Sep 17 00:00:00 2001 From: joe Date: Mon, 16 Dec 2024 18:18:48 +0000 Subject: [PATCH 1/4] Ensure game closes promptly when it crashes --- .../gametest/FabricClientGameTestRunner.java | 12 ++-- .../impl/client/gametest/ThreadingImpl.java | 63 +++++++++++++++---- .../client/gametest/MinecraftClientMixin.java | 26 ++++++-- .../client/gametest/MinecraftServerMixin.java | 24 ++++++- 4 files changed, 98 insertions(+), 27 deletions(-) diff --git a/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/FabricClientGameTestRunner.java b/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/FabricClientGameTestRunner.java index 127a21f010..5351f24809 100644 --- a/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/FabricClientGameTestRunner.java +++ b/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/FabricClientGameTestRunner.java @@ -40,15 +40,11 @@ public static void start() { for (FabricClientGameTest gameTest : gameTests) { context.restoreDefaultGameOptions(); - try { - gameTest.runTest(context); - } finally { - context.getInput().clearKeysDown(); - checkFinalGameTestState(context, gameTest.getClass().getName()); - } - } + gameTest.runTest(context); - context.clickScreenButton("menu.quit"); + context.getInput().clearKeysDown(); + checkFinalGameTestState(context, gameTest.getClass().getName()); + } }); } diff --git a/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/ThreadingImpl.java b/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/ThreadingImpl.java index a0a0a1719a..ef76ec5b5c 100644 --- a/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/ThreadingImpl.java +++ b/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/ThreadingImpl.java @@ -26,6 +26,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import net.minecraft.client.MinecraftClient; + /** *

Implementation notes

* @@ -75,6 +77,7 @@ private ThreadingImpl() { private static final int PHASE_MASK = 3; public static final Phaser PHASER = new Phaser(); + private static volatile boolean enablePhases = true; public static volatile boolean isClientRunning = false; public static volatile boolean clientCanAcceptTasks = false; @@ -87,19 +90,30 @@ private ThreadingImpl() { @Nullable public static Thread testThread = null; public static final Semaphore TEST_SEMAPHORE = new Semaphore(0); + @Nullable + public static Throwable testFailureException = null; @Nullable public static Runnable taskToRun = null; + private static volatile boolean gameCrashed = false; + public static void enterPhase(int phase) { - while ((PHASER.getPhase() & PHASE_MASK) != phase) { + while (enablePhases && (PHASER.getPhase() & PHASE_MASK) != phase) { + PHASER.arriveAndAwaitAdvance(); + } + + if (enablePhases) { PHASER.arriveAndAwaitAdvance(); } + } - PHASER.arriveAndAwaitAdvance(); + public static void setGameCrashed() { + enablePhases = false; + gameCrashed = true; } - public static void runTestThread(Runnable test) { + public static void runTestThread(Runnable testRunner) { Preconditions.checkState(testThread == null, "There is already a test thread running"); testThread = new Thread(() -> { @@ -107,28 +121,41 @@ public static void runTestThread(Runnable test) { enterPhase(PHASE_TEST); try { - test.run(); + testRunner.run(); } catch (Throwable e) { - LOGGER.error("Failed to run client gametests", e); - System.exit(1); + testFailureException = e; } finally { - PHASER.arriveAndDeregister(); - if (clientCanAcceptTasks) { - CLIENT_SEMAPHORE.release(); + runOnClient(() -> MinecraftClient.getInstance().scheduleStop()); } - if (serverCanAcceptTasks) { - SERVER_SEMAPHORE.release(); + if (testFailureException != null) { + // Log this now in case the client has stopped or is otherwise unable to rethrow our exception + LOGGER.error("Client gametests failed with an exception", testFailureException); } - testThread = null; + deregisterTestThread(); } }); testThread.setName("Test thread"); + testThread.setDaemon(true); testThread.start(); } + private static void deregisterTestThread() { + testThread = null; + enablePhases = false; + PHASER.arriveAndDeregister(); + + if (clientCanAcceptTasks) { + CLIENT_SEMAPHORE.release(); + } + + if (serverCanAcceptTasks) { + SERVER_SEMAPHORE.release(); + } + } + public static void checkOnGametestThread(String methodName) { Preconditions.checkState(Thread.currentThread() == testThread, "%s can only be called from the client gametest thread", methodName); } @@ -207,5 +234,17 @@ public static void runTick() { } enterPhase(PHASE_TEST); + + // Check if the game has crashed during this tick. If so, don't do any more work in the test + if (gameCrashed) { + deregisterTestThread(); + + try { + // wait until game is closed + new Semaphore(0).acquire(); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + } } } diff --git a/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/mixin/client/gametest/MinecraftClientMixin.java b/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/mixin/client/gametest/MinecraftClientMixin.java index 9c57131be8..bdd21f682c 100644 --- a/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/mixin/client/gametest/MinecraftClientMixin.java +++ b/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/mixin/client/gametest/MinecraftClientMixin.java @@ -50,7 +50,7 @@ public class MinecraftClientMixin { private Overlay overlay; @WrapMethod(method = "run") - private void onRun(Operation original) { + private void onRun(Operation original) throws Throwable { if (ThreadingImpl.isClientRunning) { throw new IllegalStateException("Client is already running"); } @@ -61,12 +61,21 @@ private void onRun(Operation original) { try { original.call(); } finally { - ThreadingImpl.clientCanAcceptTasks = false; - ThreadingImpl.PHASER.arriveAndDeregister(); - ThreadingImpl.isClientRunning = false; + deregisterClient(); + + if (ThreadingImpl.testFailureException != null) { + throw ThreadingImpl.testFailureException; + } } } + @Inject(method = "cleanUpAfterCrash", at = @At("HEAD")) + private void deregisterAfterCrash(CallbackInfo ci) { + // Deregister a bit earlier than normal to allow for the integrated server to stop without waiting for the client + ThreadingImpl.setGameCrashed(); + deregisterClient(); + } + @Inject(method = "tick", at = @At("HEAD")) private void onTick(CallbackInfo ci) { if (!startedClientGametests && overlay == null) { @@ -152,4 +161,13 @@ private static void checkThreadOnGetInstance(CallbackInfoReturnable original) { try { original.call(); } finally { - ThreadingImpl.serverCanAcceptTasks = false; - ThreadingImpl.PHASER.arriveAndDeregister(); - ThreadingImpl.isServerRunning = false; + deregisterServer(); } } + @Inject(method = "runServer", at = @At(value = "INVOKE", target = "Lnet/minecraft/server/MinecraftServer;setCrashReport(Lnet/minecraft/util/crash/CrashReport;)V", shift = At.Shift.AFTER)) + protected void onCrash(CallbackInfo ci) { + if (ThreadingImpl.testFailureException == null) { + ThreadingImpl.testFailureException = new Throwable("The server crashed"); + } + + MinecraftClient.getInstance().scheduleStop(); + ThreadingImpl.setGameCrashed(); + deregisterServer(); + } + @Inject(method = "runServer", at = @At(value = "INVOKE", target = "Lnet/minecraft/server/MinecraftServer;runTasksTillTickEnd()V")) private void preRunTasks(CallbackInfo ci) { ThreadingImpl.enterPhase(ThreadingImpl.PHASE_SERVER_TASKS); @@ -78,4 +89,11 @@ private void postRunTasks(CallbackInfo ci) { ThreadingImpl.enterPhase(ThreadingImpl.PHASE_TICK); } + + @Unique + private void deregisterServer() { + ThreadingImpl.serverCanAcceptTasks = false; + ThreadingImpl.PHASER.arriveAndDeregister(); + ThreadingImpl.isServerRunning = false; + } } From 3dfdebf6240240961645d9cf58fda694a532a64d Mon Sep 17 00:00:00 2001 From: joe Date: Mon, 16 Dec 2024 19:31:25 +0000 Subject: [PATCH 2/4] Mutate the stack traces of exceptions thrown in tasks to make them easier to track --- .../api/client/gametest/v1/package-info.java | 7 +- .../impl/client/gametest/ThreadingImpl.java | 105 ++++++++++++------ 2 files changed, 77 insertions(+), 35 deletions(-) diff --git a/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/api/client/gametest/v1/package-info.java b/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/api/client/gametest/v1/package-info.java index 79497dca5d..521dd389e4 100644 --- a/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/api/client/gametest/v1/package-info.java +++ b/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/api/client/gametest/v1/package-info.java @@ -11,8 +11,11 @@ * *

Client gametests run on the client gametest thread. Use the functions inside * {@link net.fabricmc.fabric.api.client.gametest.v1.ClientGameTestContext ClientGameTestContext} and other test helper - * classes to run code on the correct thread. The game remains paused unless you explicitly unpause it using various - * waiting functions such as + * classes to run code on the correct thread. Exceptions are transparently rethrown on the test thread, and their stack + * traces are mutated to include the async stack trace, to make them easy to track. You can disable this behavior by + * setting the {@code fabric.client.gametest.disableJoinAsyncStackTraces} system property. + * + *

The game remains paused unless you explicitly unpause it using various waiting functions such as * {@link net.fabricmc.fabric.api.client.gametest.v1.ClientGameTestContext#waitTick() ClientGameTestContext.waitTick()}. * *

A few changes have been made to how the vanilla game threads run, to make tests more reproducible. Notably, there diff --git a/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/ThreadingImpl.java b/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/ThreadingImpl.java index ef76ec5b5c..765f21e127 100644 --- a/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/ThreadingImpl.java +++ b/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/ThreadingImpl.java @@ -70,6 +70,11 @@ private ThreadingImpl() { private static final Logger LOGGER = LoggerFactory.getLogger("fabric-client-gametest-api-v1"); + private static final boolean DISABLE_JOIN_ASYNC_STACK_TRACES = System.getProperty("fabric.client.gametest.disableJoinAsyncStackTraces") != null; + private static final String THREAD_IMPL_CLASS_NAME = ThreadingImpl.class.getName(); + private static final String TASK_ON_THIS_THREAD_METHOD_NAME = "runTaskOnThisThread"; + private static final String TASK_ON_OTHER_THREAD_METHOD_NAME = "runTaskOnOtherThread"; + public static final int PHASE_TICK = 0; public static final int PHASE_SERVER_TASKS = 1; public static final int PHASE_CLIENT_TASKS = 2; @@ -160,25 +165,25 @@ public static void checkOnGametestThread(String methodName) { Preconditions.checkState(Thread.currentThread() == testThread, "%s can only be called from the client gametest thread", methodName); } - @SuppressWarnings("unchecked") public static void runOnClient(FailableRunnable action) throws E { Preconditions.checkNotNull(action, "action"); checkOnGametestThread("runOnClient"); Preconditions.checkState(clientCanAcceptTasks, "runOnClient called when no client is running"); + runTaskOnOtherThread(action, CLIENT_SEMAPHORE); + } + public static void runOnServer(FailableRunnable action) throws E { + Preconditions.checkNotNull(action, "action"); + checkOnGametestThread("runOnServer"); + Preconditions.checkState(serverCanAcceptTasks, "runOnServer called when no server is running"); + runTaskOnOtherThread(action, SERVER_SEMAPHORE); + } + + private static void runTaskOnOtherThread(FailableRunnable action, Semaphore clientOrServerSemaphore) throws E { MutableObject thrown = new MutableObject<>(); - taskToRun = () -> { - try { - action.run(); - } catch (Throwable e) { - thrown.setValue((E) e); - } finally { - taskToRun = null; - TEST_SEMAPHORE.release(); - } - }; + taskToRun = () -> runTaskOnThisThread(action, thrown); - CLIENT_SEMAPHORE.release(); + clientOrServerSemaphore.release(); try { TEST_SEMAPHORE.acquire(); @@ -187,39 +192,73 @@ public static void runOnClient(FailableRunnable action) } if (thrown.getValue() != null) { + joinAsyncStackTrace(thrown.getValue()); throw thrown.getValue(); } } @SuppressWarnings("unchecked") - public static void runOnServer(FailableRunnable action) throws E { - Preconditions.checkNotNull(action, "action"); - checkOnGametestThread("runOnServer"); - Preconditions.checkState(serverCanAcceptTasks, "runOnServer called when no server is running"); + private static void runTaskOnThisThread(FailableRunnable action, MutableObject thrown) { + try { + action.run(); + } catch (Throwable e) { + thrown.setValue((E) e); + } finally { + taskToRun = null; + TEST_SEMAPHORE.release(); + } + } - MutableObject thrown = new MutableObject<>(); - taskToRun = () -> { - try { - action.run(); - } catch (Throwable e) { - thrown.setValue((E) e); - } finally { - taskToRun = null; - TEST_SEMAPHORE.release(); + private static void joinAsyncStackTrace(Throwable e) { + if (DISABLE_JOIN_ASYNC_STACK_TRACES) { + return; + } + + // find the end of the relevant part of the stack trace on the other thread + StackTraceElement[] taskStackTrace = e.getStackTrace(); + + if (taskStackTrace == null) { + return; + } + + int taskRunOnThreadIndex = taskStackTrace.length - 1; + + for (; taskRunOnThreadIndex >= 0; taskRunOnThreadIndex--) { + StackTraceElement element = taskStackTrace[taskRunOnThreadIndex]; + + if (THREAD_IMPL_CLASS_NAME.equals(element.getClassName()) && TASK_ON_THIS_THREAD_METHOD_NAME.equals(element.getMethodName())) { + break; } - }; + } - SERVER_SEMAPHORE.release(); + if (taskRunOnThreadIndex == -1) { + // couldn't find stack trace element + return; + } - try { - TEST_SEMAPHORE.acquire(); - } catch (InterruptedException e) { - throw new RuntimeException(e); + // find the start of the relevant part of the stack trace on the test thread + StackTraceElement[] testStackTrace = Thread.currentThread().getStackTrace(); + int testRunOnThreadIndex = 0; + + for (; testRunOnThreadIndex < testStackTrace.length; testRunOnThreadIndex++) { + StackTraceElement element = testStackTrace[testRunOnThreadIndex]; + + if (THREAD_IMPL_CLASS_NAME.equals(element.getClassName()) && TASK_ON_OTHER_THREAD_METHOD_NAME.equals(element.getMethodName())) { + break; + } } - if (thrown.getValue() != null) { - throw thrown.getValue(); + if (testRunOnThreadIndex == testStackTrace.length) { + // couldn't find stack trace element + return; } + + // join the stack traces + StackTraceElement[] joinedStackTrace = new StackTraceElement[(taskRunOnThreadIndex + 1) + 1 + (testStackTrace.length - taskRunOnThreadIndex)]; + System.arraycopy(taskStackTrace, 0, joinedStackTrace, 0, taskRunOnThreadIndex + 1); + joinedStackTrace[taskRunOnThreadIndex + 1] = new StackTraceElement("Async Stack Trace", ".", null, 1); + System.arraycopy(testStackTrace, testRunOnThreadIndex, joinedStackTrace, taskRunOnThreadIndex + 2, testStackTrace.length - testRunOnThreadIndex); + e.setStackTrace(joinedStackTrace); } public static void runTick() { From db066d57874be1a50820fefc8736e7af7f44300a Mon Sep 17 00:00:00 2001 From: Joe Date: Tue, 17 Dec 2024 13:07:00 +0000 Subject: [PATCH 3/4] Fix ThreadingImpl.joinAsyncStackTrace --- .../impl/client/gametest/ThreadingImpl.java | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/ThreadingImpl.java b/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/ThreadingImpl.java index 765f21e127..4045408f15 100644 --- a/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/ThreadingImpl.java +++ b/fabric-client-gametest-api-v1/src/client/java/net/fabricmc/fabric/impl/client/gametest/ThreadingImpl.java @@ -215,49 +215,49 @@ private static void joinAsyncStackTrace(Throwable e) { } // find the end of the relevant part of the stack trace on the other thread - StackTraceElement[] taskStackTrace = e.getStackTrace(); + StackTraceElement[] otherThreadStackTrace = e.getStackTrace(); - if (taskStackTrace == null) { + if (otherThreadStackTrace == null) { return; } - int taskRunOnThreadIndex = taskStackTrace.length - 1; + int otherThreadIndex = otherThreadStackTrace.length - 1; - for (; taskRunOnThreadIndex >= 0; taskRunOnThreadIndex--) { - StackTraceElement element = taskStackTrace[taskRunOnThreadIndex]; + for (; otherThreadIndex >= 0; otherThreadIndex--) { + StackTraceElement element = otherThreadStackTrace[otherThreadIndex]; if (THREAD_IMPL_CLASS_NAME.equals(element.getClassName()) && TASK_ON_THIS_THREAD_METHOD_NAME.equals(element.getMethodName())) { break; } } - if (taskRunOnThreadIndex == -1) { + if (otherThreadIndex == -1) { // couldn't find stack trace element return; } // find the start of the relevant part of the stack trace on the test thread - StackTraceElement[] testStackTrace = Thread.currentThread().getStackTrace(); - int testRunOnThreadIndex = 0; + StackTraceElement[] thisThreadStackTrace = Thread.currentThread().getStackTrace(); + int thisThreadIndex = 0; - for (; testRunOnThreadIndex < testStackTrace.length; testRunOnThreadIndex++) { - StackTraceElement element = testStackTrace[testRunOnThreadIndex]; + for (; thisThreadIndex < thisThreadStackTrace.length; thisThreadIndex++) { + StackTraceElement element = thisThreadStackTrace[thisThreadIndex]; if (THREAD_IMPL_CLASS_NAME.equals(element.getClassName()) && TASK_ON_OTHER_THREAD_METHOD_NAME.equals(element.getMethodName())) { break; } } - if (testRunOnThreadIndex == testStackTrace.length) { + if (thisThreadIndex == thisThreadStackTrace.length) { // couldn't find stack trace element return; } // join the stack traces - StackTraceElement[] joinedStackTrace = new StackTraceElement[(taskRunOnThreadIndex + 1) + 1 + (testStackTrace.length - taskRunOnThreadIndex)]; - System.arraycopy(taskStackTrace, 0, joinedStackTrace, 0, taskRunOnThreadIndex + 1); - joinedStackTrace[taskRunOnThreadIndex + 1] = new StackTraceElement("Async Stack Trace", ".", null, 1); - System.arraycopy(testStackTrace, testRunOnThreadIndex, joinedStackTrace, taskRunOnThreadIndex + 2, testStackTrace.length - testRunOnThreadIndex); + StackTraceElement[] joinedStackTrace = new StackTraceElement[(otherThreadIndex + 1) + 1 + (thisThreadStackTrace.length - thisThreadIndex)]; + System.arraycopy(otherThreadStackTrace, 0, joinedStackTrace, 0, otherThreadIndex + 1); + joinedStackTrace[otherThreadIndex + 1] = new StackTraceElement("Async Stack Trace", ".", null, 1); + System.arraycopy(thisThreadStackTrace, thisThreadIndex, joinedStackTrace, otherThreadIndex + 2, thisThreadStackTrace.length - thisThreadIndex); e.setStackTrace(joinedStackTrace); } From 3bd2369fee827831c63ad0255f1d0e96330aa7c5 Mon Sep 17 00:00:00 2001 From: Joe Date: Wed, 18 Dec 2024 11:53:24 +0000 Subject: [PATCH 4/4] Don't press menu.quit button from inside the gametest test --- .../fabricmc/fabric/test/client/gametest/ClientGameTestTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/fabric-client-gametest-api-v1/src/testmodClient/java/net/fabricmc/fabric/test/client/gametest/ClientGameTestTest.java b/fabric-client-gametest-api-v1/src/testmodClient/java/net/fabricmc/fabric/test/client/gametest/ClientGameTestTest.java index 07d684e2db..61956ad959 100644 --- a/fabric-client-gametest-api-v1/src/testmodClient/java/net/fabricmc/fabric/test/client/gametest/ClientGameTestTest.java +++ b/fabric-client-gametest-api-v1/src/testmodClient/java/net/fabricmc/fabric/test/client/gametest/ClientGameTestTest.java @@ -143,7 +143,6 @@ public void runTest(ClientGameTestContext context) { { context.waitForScreen(TitleScreen.class); - context.clickScreenButton("menu.quit"); } }