Implement changes to client auto-test threading #4256
Conversation
...ase/src/testmodClient/java/net/fabricmc/fabric/test/base/client/FabricApiAutoTestClient.java
Outdated
Show resolved
Hide resolved
…ts, reintroduce MinecraftClient parameter
.../src/testmodClient/java/net/fabricmc/fabric/test/base/client/mixin/MinecraftClientMixin.java
Outdated
Show resolved
Hide resolved
| public static <T, E extends Throwable> T computeOnClient(FailableFunction<MinecraftClient, T, E> action) throws E { | ||
| MutableObject<T> result = new MutableObject<>(); | ||
| runOnClient(minecraft -> result.setValue(action.apply(minecraft))); | ||
| return result.getValue(); | ||
| } |
There was a problem hiding this comment.
Suggestion: Calling it getFromClient() instead of computeOnClient() would make it clearer that this method returns a value while runOnClient() doesn't.
Your use of compute here isn't wrong, I believe the Java API does something similar, but I think many people use run and compute synonymously and wouldn't immediately grasp that this is the difference between them. get vs run makes it really obvious.
There was a problem hiding this comment.
I'm pretty sure this is the more common terminology, although I'm not super attached to it. But it should be immediately obvious what the difference between the two functions are from the parameter type
There was a problem hiding this comment.
I do like simple language, but yeah compute is pretty common too. I don't think parameter/return types are usually the first thing I would look at when working with a new API, but then again this will eventually have Javadoc that can make it obvious more quickly.
.../src/testmodClient/java/net/fabricmc/fabric/test/base/client/mixin/MinecraftClientMixin.java
Outdated
Show resolved
Hide resolved
| private <T> CompletableFuture<T> submit(Function<MinecraftDedicatedServer, T> function) { | ||
| return server.submit(() -> function.apply(server)); | ||
| } | ||
|
|
||
| private <T> T submitAndWait(Function<MinecraftDedicatedServer, T> function) { | ||
| return submit(function).join(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are there no runOnServer() / computeOnServer() helpers here? I mean, I can't say that I've used the old TestDedicatedServer.submitAndWait() in actual test code, but this seems like something that more server-heavy mods would find useful in their tests. Obviously one can run ThreadingImpl.runOnServer() directly instead, as you've done above, but it feels inconsistent to have these helpers on one side and not the other.
There was a problem hiding this comment.
They were private and unused so I deleted them, the equivalent is in ThreadingImpl. We can think about what helpers are useful to expose in a future PR
| public static Runnable taskToRun = null; | ||
|
|
||
| public static void enterPhase(int phase) { | ||
| while ((PHASER.getPhase() & PHASE_MASK) != phase) { |
There was a problem hiding this comment.
This is the only bit that is midly confusing to me (I think), is this method blocking until the current phase has been reached from somewhere else? My tottal lack of knowledge of Phaser doesnt help
There was a problem hiding this comment.
This method blocks until the given tick phase has been reached on all threads registered to the Phaser ("parties"), while allowing other parties to advance through the other tick phases (which is what the loop is for). The arriveAndAwaitAdvance is a barrier that all parties must call before they are all unblocked at once.
A Phaser maintains a counter called the "phase" which increments each time a barrier is crossed. It starts at 0 and when it surpasses Integer.MAX_VALUE it resets back to 0 again (negative values happen when the Phaser is terminated, which is not functionality we're using). Using the lower 2 bits of this counter that already exists tells us which of our 4 tick phases we're currently in. It's possible to track that separately by subclassing the Phaser and overriding its onAdvance method, but I figured why not take advantage of a counter that already exists.
Implement changes to client auto-test threading as discussed on Discord. More detailed implementation notes can be found on the docs to
ThreadingImpl.I didn't put much thought into class naming, or what should or shouldn't be in the API, etc, as this is all still internal, and deciding on a nice clean API is left for future work.