[1.16.5] Properly using DistExecutor with arguments

[Choonster 1651]

I have a packet that's sent to the client to open a GUI, which I'm using DistExecutor to do.

 

The packet's handler method does the following:

DistExecutor.safeRunWhenOn(Dist.CLIENT, () -> ClientOnlyNetworkMethods.openClientScreen(message))

 

ClientOnlyNetworkMethods.openClientScreen currently looks like this:

public static DistExecutor.SafeRunnable openClientScreen(final OpenClientScreenMessage message) {
	return new DistExecutor.SafeRunnable() {
		@Override
		public void run() {
			ClientScreenManager.openScreen(message.getId(), message.getAdditionalData(), Minecraft.getInstance());
		}
	};
}

 

ClientScreenManager is a client-only class that handles opening the GUI.

 

As you can see from the code, I need to pass arguments from the packet to the client-only method; which rules out using a method reference as the SafeRunnable implementation.

 

When I replace the anonymous class implementation of SafeRunnable in ClientOnlyNetworkMethods.openClientScreen with a lambda, DistExecutor.validateSafeReferent throws an "Unsafe Referent usage found in safe referent method" exception. From what I can see, using any non-lambda implementation of SafeReferent simply bypasses the safety checks in validateSafeReferent but doesn't necessarily mean that the code is safe.

 

The current code with the anonymous class does seem to work on the dedicated server, but is this the correct way to use DistExecutor; or is there a better way to do it?

Edited January 23 by Choonster

[ChampionAsh5357 165]

From what I understand, this is not the correct way to use DistExecutor. For the case where you can't supply a runnable or supplier, DistExecutor#unsafe* should be used instead. This will supply a runnable of what you want to execute (e.g. () -> () -> //Do things). This does not verify nor guarantee that the code is completely safe to access; however, if the runnable executes another method that is isolated in a different class, it is 'safe' since classloading will not occur. So, the proper way to implement the code above is DistExecutor.unsafeRunWhenOn(Dist.CLIENT, () -> () -> ClientScreenManager#openScreen).

[Choonster 1651]

19 hours ago, ChampionAsh5357 said:

From what I understand, this is not the correct way to use DistExecutor. For the case where you can't supply a runnable or supplier, DistExecutor#unsafe* should be used instead. This will supply a runnable of what you want to execute (e.g. () -> () -> //Do things). This does not verify nor guarantee that the code is completely safe to access; however, if the runnable executes another method that is isolated in a different class, it is 'safe' since classloading will not occur. So, the proper way to implement the code above is DistExecutor.unsafeRunWhenOn(Dist.CLIENT, () -> () -> ClientScreenManager#openScreen).

 

Thanks, I think that makes sense.

 

I've tried to follow this advice and clean up all my DistExecutor code in this commit, does this look correct?

[ChampionAsh5357 165]

18 minutes ago, Choonster said:

I've tried to follow this advice and clean up all my DistExecutor code in this commit, does this look correct?

My only comment would be on OpenClientScreenMessage as the Minecraft instance can just be obtained inside the method itself. This is still 'safe' I believe, but we should avoid calling anything that might only be available on the client that is not isolated in a different class. This is my opinion as it still won't be loaded unless on the physical client.

[Choonster 1651]

On 1/25/2021 at 1:47 AM, ChampionAsh5357 said:

My only comment would be on OpenClientScreenMessage as the Minecraft instance can just be obtained inside the method itself. This is still 'safe' I believe, but we should avoid calling anything that might only be available on the client that is not isolated in a different class. This is my opinion as it still won't be loaded unless on the physical client.

 

Thanks, that makes sense.