chore: CoreRegistry removal from engine.network #5033

keturn · 2022-05-30T02:48:35Z

Fixes some race conditions in MTE tests.

How to test

Test all the networking things!

Join server, leave server, browse server list, etc.

keturn · 2022-05-30T03:05:13Z

After asking #gestalt and seeing some constructor injector usage with the upcoming gestalt-di, I went with that approach. It felt safer than field injection, as I know I can't create an instance without filling those values this way.

Fixes some race conditions in MTE tests. test(network): ClientHandler's GameEngine reference is optional? seems sus, but it's only used for a disconnection event, so maybe.

keturn · 2022-05-30T23:31:21Z

got some problems using this as a server; switching to Draft while I figure that out

with some documentation

keturn · 2022-05-31T00:27:37Z

okay, that was one part boring NPE and one part not knowing how to run a server

skaldarnar

Didn't test it yet, but code looks good to me 👍

Left a few questions, but none of them are really blocking.

skaldarnar · 2022-06-04T11:40:38Z

engine/src/main/java/org/terasology/engine/core/modes/StateLoading.java

@@ -110,6 +111,7 @@ public void init(GameEngine engine) {
        headless = context.get(DisplayDevice.class).isHeadless();

        CoreRegistry.setContext(context);
+        context.getValue(NetworkSystem.class).setContext(context);


What does this do? We are getting the network system out of the context, and then set the context for that network system?

Does it create a "back link" from the network system to the context it lives in? Or is the network system distributing the context further?

See

Terasology/engine/src/main/java/org/terasology/engine/network/internal/NetworkSystemImpl.java

Lines 778 to 786 in 6647a4e

/**

* Sets the context within which this system should operate.

* <p>

* As a client transitions from connecting to loading to in-game, it moves

* through different {@link org.terasology.engine.core.modes.GameState GameStates},

* and the context changes along the way.

*/

@Override

public void setContext(Context newContext) {

.

I'm not 100% certain this addition is necessary, as I put it in during some troubleshooting and didn't confirm later. But it's near where the context is created, and it's being set in CoreRegistry which NetworkSystem was using, so it's probably a good idea.

skaldarnar · 2022-06-04T11:41:44Z

engine/src/main/java/org/terasology/engine/core/subsystem/common/NetworkSubsystem.java

@@ -23,7 +24,7 @@ public String getName() {

    @Override
    public void initialise(GameEngine engine, Context rootContext) {
-        networkSystem = new NetworkSystemImpl(rootContext.get(Time.class), rootContext);
+        networkSystem = new NetworkSystemImpl((EngineTime) rootContext.get(Time.class), rootContext);


This looks ... unsafe 🤔 If it is important that it is an EngineTime, why don't we put it in the context as EngineTime instead of Time?

yeah. I wondered that too.

My best guess is that Time is the read-only interface, and the one that gets put in Context for most common use, while EngineTime has other setter methods on it that the network system needs to synchronize time between client and server.

But as we can see, "please don't cast this" is not an effective form of capability control, so 🤷

skaldarnar · 2022-06-04T12:00:03Z

engine/src/main/java/org/terasology/engine/network/internal/ClientHandler.java

-    public ClientHandler(NetworkSystemImpl networkSystem) {
+    @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
+    public ClientHandler(NetworkSystemImpl networkSystem, Optional<GameEngine> gameEngine) {
+        this.gameEngine = gameEngine.orElse(null);


How does this behave if the engine actually happens to be `null? Will we get a reasonable error message?

Its only usage is here:

Terasology/engine/src/main/java/org/terasology/engine/network/internal/ClientHandler.java

Lines 37 to 39 in 6647a4e

public void channelInactive(ChannelHandlerContext ctx) throws Exception {

if (gameEngine != null) {

gameEngine.changeState(new StateMainMenu("Disconnected From Server"));

I still think it's a little suspect, but it's the nearest replacement to how CoreRegistry.get works.

keturn

I think we'll have to ask SpotBugs to chill with the EI_EXPOSE_REP2

keturn · 2022-06-04T16:09:17Z

engine/src/main/java/org/terasology/engine/core/subsystem/common/NetworkSubsystem.java

@@ -23,7 +24,7 @@ public String getName() {

    @Override
    public void initialise(GameEngine engine, Context rootContext) {
-        networkSystem = new NetworkSystemImpl(rootContext.get(Time.class), rootContext);
+        networkSystem = new NetworkSystemImpl((EngineTime) rootContext.get(Time.class), rootContext);


yeah. I wondered that too.

My best guess is that Time is the read-only interface, and the one that gets put in Context for most common use, while EngineTime has other setter methods on it that the network system needs to synchronize time between client and server.

But as we can see, "please don't cast this" is not an effective form of capability control, so 🤷

keturn · 2022-06-04T16:12:03Z

engine/src/main/java/org/terasology/engine/network/internal/ClientHandler.java

-    public ClientHandler(NetworkSystemImpl networkSystem) {
+    @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
+    public ClientHandler(NetworkSystemImpl networkSystem, Optional<GameEngine> gameEngine) {
+        this.gameEngine = gameEngine.orElse(null);


Its only usage is here:

Terasology/engine/src/main/java/org/terasology/engine/network/internal/ClientHandler.java

Lines 37 to 39 in 6647a4e

public void channelInactive(ChannelHandlerContext ctx) throws Exception {

if (gameEngine != null) {

gameEngine.changeState(new StateMainMenu("Disconnected From Server"));

I still think it's a little suspect, but it's the nearest replacement to how CoreRegistry.get works.

…egistry # Conflicts: # engine/src/main/java/org/terasology/engine/context/internal/ContextImpl.java

skaldarnar

Tested in local multiplay with a couple of bugs (which are also present on develop). Could connect just fine, so LGTM.

keturn added Multiplayer Affects aspects not visible in Singleplayer mode only Size: M Medium-sized effort likely requiring some research and work in multiple areas Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness labels May 30, 2022

github-actions bot added the Type: Chore Request for or implementation of maintenance changes label May 30, 2022

keturn force-pushed the chore/networkCoreRegistry branch from 97a6b0e to 6b569ae Compare May 30, 2022 02:56

keturn mentioned this pull request May 30, 2022

feat(InjectionHelper): inject() returns the object for chaining #5028

Merged

chore: CoreRegistry removal from engine.network

da898bc

Fixes some race conditions in MTE tests. test(network): ClientHandler's GameEngine reference is optional? seems sus, but it's only used for a disconnection event, so maybe.

keturn force-pushed the chore/networkCoreRegistry branch from 6b569ae to da898bc Compare May 30, 2022 04:32

keturn marked this pull request as draft May 30, 2022 23:30

fix(network): don't replace this object every tick

6647a4e

with some documentation

keturn force-pushed the chore/networkCoreRegistry branch from 23c186e to 6647a4e Compare May 31, 2022 00:27

keturn marked this pull request as ready for review May 31, 2022 00:27

keturn mentioned this pull request Jun 1, 2022

test: add NetworkMode setting to MTE #5037

Merged

skaldarnar reviewed Jun 4, 2022

View reviewed changes

keturn commented Jun 4, 2022

View reviewed changes

Merge remote-tracking branch 'origin/develop' into chore/networkCoreR…

10eabd7

…egistry # Conflicts: # engine/src/main/java/org/terasology/engine/context/internal/ContextImpl.java

jdrueckert added the Status: Needs Testing Requires to be tested in-game for reproducibility label Jun 4, 2022

jdrueckert added 2 commits June 4, 2022 19:57

Merge branch 'develop' into chore/networkCoreRegistry

2fba368

Merge branch 'develop' into chore/networkCoreRegistry

e2ba1b2

skaldarnar approved these changes Jun 6, 2022

View reviewed changes

skaldarnar merged commit be0f9de into develop Jun 6, 2022

skaldarnar deleted the chore/networkCoreRegistry branch June 6, 2022 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: CoreRegistry removal from engine.network #5033

chore: CoreRegistry removal from engine.network #5033

keturn commented May 30, 2022 •

edited

Loading

keturn commented May 30, 2022

keturn commented May 30, 2022

keturn commented May 31, 2022

skaldarnar left a comment

skaldarnar Jun 4, 2022

keturn Jun 4, 2022

skaldarnar Jun 4, 2022

keturn Jun 4, 2022

skaldarnar Jun 4, 2022

keturn Jun 4, 2022

keturn left a comment

keturn Jun 4, 2022

keturn Jun 4, 2022

skaldarnar left a comment

	/**
	* Sets the context within which this system should operate.
	* <p>
	* As a client transitions from connecting to loading to in-game, it moves
	* through different {@link org.terasology.engine.core.modes.GameState GameStates},
	* and the context changes along the way.
	*/
	@Override
	public void setContext(Context newContext) {

	public void channelInactive(ChannelHandlerContext ctx) throws Exception {
	if (gameEngine != null) {
	gameEngine.changeState(new StateMainMenu("Disconnected From Server"));

chore: CoreRegistry removal from engine.network #5033

chore: CoreRegistry removal from engine.network #5033

Conversation

keturn commented May 30, 2022 • edited Loading

How to test

keturn commented May 30, 2022

keturn commented May 30, 2022

keturn commented May 31, 2022

skaldarnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keturn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skaldarnar left a comment

Choose a reason for hiding this comment

keturn commented May 30, 2022 •

edited

Loading