Don't spawn new process when joining host in lobby

[RoiEXLab]

Change Summary & Additional Notes

@asvitkine @DanVanAtta Please have a look at this changes. I think this change might be fine 90% of the time, but for the other 10% this might introduce unexpected behaviour. I don't trust the code enough to perform the same changes for hosting games on the lobby because there's some game logic that just exists the JVM on certain conditions. I'd really like to hear your opinion on this. If you think this is a good choice or if some further refactoring is required before this can happen.

Release Note

CHANGE|TripleA no longer creates a new process when joining a game in the lobby

[codecov[bot]]

Codecov Report

Merging #10747 (96e6608) into master (19d4d3c) will decrease coverage by 0.00%. The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master   #10747      +/-   ##
============================================
- Coverage     27.33%   27.32%   -0.01%     
+ Complexity     8270     8266       -4     
============================================
  Files          1217     1217              
  Lines         77805    77790      -15     
  Branches      10588    10587       -1     
============================================
- Hits          21269    21259      -10     
+ Misses        54575    54570       -5     
  Partials       1961     1961              

Impacted Files	Coverage Δ
...ategy/engine/framework/startup/mc/ClientModel.java	0.90% <0.00%> (+0.03%)	:arrow_up:
...ames/strategy/engine/auto/update/UpdateChecks.java	0.00% <ø> (ø)
...a/games/strategy/engine/framework/GameProcess.java	0.00% <ø> (ø)
...gy/engine/framework/startup/ui/MetaSetupPanel.java	0.00% <0.00%> (ø)
...startup/ui/panels/main/HeadedServerSetupModel.java	0.00% <0.00%> (ø)
...trategy/engine/lobby/client/ui/LobbyGamePanel.java	0.00% <0.00%> (ø)
...java/org/triplea/game/client/HeadedGameRunner.java	0.00% <0.00%> (ø)
.../src/main/java/games/strategy/net/nio/Decoder.java	74.13% <0.00%> (-5.18%)	:arrow_down:
...rc/main/java/games/strategy/net/nio/NioReader.java	82.50% <0.00%> (-3.75%)	:arrow_down:
...lea/ai/flowfield/influence/InfluenceTerritory.java	96.96% <0.00%> (-3.04%)	:arrow_down:
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 19d4d3c...96e6608. Read the comment docs.

[DanVanAtta]

@RoiEXLab sounds like a good direction, what am I missing? As you hinted at, my only concern is the 'accuracy/safety' of the update - it should be solid and needs to work for all the times (or at least as often as it does now)

[RoiEXLab]

Ah sorry if i wasn't clear enough. The change works as-is but I just don't know if there's any situation where a system.exit call closes too many windows or if there's a situation where you can open multiple games at the same time that interfere with each other. Spawning a new process does have the benefit of strict memory separation after all.

[RoiEXLab]

@asvitkine @DanVanAtta Your call if you want to merge this. I believe it should work but I can't guarantee it for every potential edge case

[DanVanAtta]

Can we put this behind a feature flag @RoiEXLab ? How much more testing would be needed to be reasonably confident this is okay?

[RoiEXLab]

@DanVanAtta A feature flag is tricky to implement here. The change of this PR allows us to drop the logic that disables update checks on starts, so we'd have to introduce that.

How much more testing would be needed to be reasonably confident this is okay?

To be honest I think it needs to be out in the wild to see if something breaks. I checked common things like leaving hosted games and so on, but I don't know what happens if you manage to have a game open already and join a game via the lobby afterwards. No idea what peoples use-cases are here. So to be clear I don't think there will be game-breaking regressions here, I'm just concerned there might be cases where users get upset because the whole application stopped even though they just wanted to end their game (also not sure if multiple simultaneous games within the same process are a thing, it would make a difference there). If this is merged I just don't want to be the guy solely responsible for this, I want this to be a group decision.

[DanVanAtta]

@RoiEXLab one thing that came to mind - what happens if a person joins many lobby games?

[RoiEXLab]

@DanVanAtta Actually I'm not entirely sure. I'm pretty sure it will cause problems because of many singletons in the code, but I'm not sure if this change even allows you to join multiple games

[DanVanAtta]

Not being able to join multiple games would be a significant behavior change. I think that is probably desirable to avoid having to relaunch the game and redo a login to lobby. When we can get it so that multiple games can be joined with the same process, we may want to put a limit to avoid too many resources from being used.

It's probably worth running this by the community first @RoiEXLab if you think this would be a good update to have. From a code hygiene perspective, it's a very good update. From a behavioral perspective, it might be viewed as a regression.

[RoiEXLab]

When I have the time I'll have another look at this. I think the underlying problem boils down to inconsistent behaviour of the launching UI. In single player the main UI always hides itself when the game starts IIRC. I assume with this change would cause this behaviour to apply to lobby games as well. (I haven't tested this extensively, but I'm pretty sure this is pretty close to the actual behaviour minus some edge cases.) That being said I'm not sure what the right move would be here? Should locally hosted games keep the Window open for the duration of the game so we can have consistent behaviour independent of game mode? How would the post game screen behave in this case of two games end at the same time? This is more of a UX design issue rather than a code issue in my opinion.

An alternative if fixing the UX seems infeasible that could be a sort of middle ground would be to create a separate entry point for the code. This second entry point would skip stuff like update check by not calling the relevant methods. This way we could reduce complexity by not having if checks left and right, but we'd still spawn an extra process which makes attaching a debugger harder. Thoughts?

[DanVanAtta]

A-priori, I closed this just to get it off of the active list. Feel free to convert to a draft & re-open @RoiEXLab

Should locally hosted games keep the Window open for the duration of the game so we can have consistent behaviour independent of game mode?

IMO no, in a single player game a person does not really need the launcher screen anymore.

This is more of a UX design issue rather than a code issue in my opinion.

Agree

Thoughts?

Modularity - "game-headed" perhaps can be merged into "game-core"

I've thought the service-loader logic we have between game-headed & game-core is quite complex. 'game-headless' I think has its days limited once we introduce the network relay pattern. It also seems like creating modules "off the top" rather than "from teh bottom" has been not very successful. To explain, the map-parsing module is a low-level module used by game-core. The map-parsing module is relatively complete & seems like a good modularization. In contrast, 'game-headed' is extremely thin and does not seem to provide a lot of benefit.

So, I think we probably could combine the 'game-headed' module into game-core.

Multiple Main Methods

Some other thoughts:

• having a config file might be nicer going forward compared to using CLI arguments. There is nothing really bad about invoking a main method directly from in-code. So instead of doing an System exec, it's very possible just to call the static main method.

Perhaps this is a case where SRP comes into play. Instead having one method do everything based on switches and flags, there could be multiple main methods. That might simplify things a bit to simply invoke a different main to launch the game vs starting up the launch screens. That should do away with quite a few of the flags and make things more straight forward.

Statics are not necessarily bad, but perhaps need to be keyed by Game-ID

Having some data & game state available via a static context I think is good. That is not to say we should abandon dependency injection, but use the two in a good way without a lot of static coupling. Basically along the lines of how Injections was a static class, but instances from the class were passed to constructors, or functions to get those instances were passed into classes. The reason I think this is desirable is to avoid the long-chained wiring of instance classes deep into the stack. The latter creates a lot of method API pollution, lots of indirection and data passing for the sake of passing data deeper into the stack.

Beyond making good use of statics, to support multiple instances, I think a lot of the static data will need to be keyed by a 'game-id' in order to support multiple games. So instead of GameDataContainer.getCurrentGameData(), we'd have something like GameDataContainer.getCurrentGameData(gameId).

[RoiEXLab]

I don't quite agree on your view of the game-headless module. If we're ready to drop the headless code entirely because we found an alternative you're right that we technically don't need the separation anymore. However I believe the separation helps splitting UI implementation details from actual logic. For example we could forbid using the javax.swing package in a pure game logic module in the future which could be a mechanism to ensure porting the engine to different UI frameworks stays feasible. Of course this is not a realistic goal for now, but in my opinion that doesn't mean there's no benefit in trying.

About static vs non-static: I kind of agree. There are definitely cases where static accessors make sense. I believe a good rule of thumb is that static is a good choice when dealing with things that are guaranteed to be unique (i.e. access to an I/O resource). However this property cannot be applied transitively, so for the analogy reading a file can be static, but if you handle the whole processing of a config file in a static way as well this makes testing really hard because the layers are tightly coupled

[DanVanAtta]

I think keeping game-headless split is okay since it'll hopefully be expediently deleted in 2.7

The split of game-headed from game-core just has not been working out. With the benefit of hindsight, there just is almost no code in game-headed and it's seemingly super difficult to move code "up" from game-core into game-headed. I think we need to approach this by dropping code "down" from game-core. So rather than moving logic "up" from game-core, it might be a better effort to move the UI code "down" into a game-ui that game-core then depends on. I'm thinking we are probably missing a module called something like app-state that would represent the meta-state of the application (eg: in launchers, game is running, which screen is current)

Overall, I do think the game-headed split has been a bit of a failure. I'm not against trying, just it might be more fruitful to merge it back into game-core and attempt the split in another manner.

re: About static vs non-static

I'll emphasize there is a huge difference between static access & static coupling. On the flip side, adding a parameter to 5 layers of method calls to pass data around is really bad. I'm a fan of the "imperative shell, functional core" architecture, the imperative shell seemingly is a good place to grab data that can very well be accessed in a static manner to then pass to the functional core. I'm thinking of things like the 'network bridge', that is a PITA to pass around everywhere that needs network access. Instead it would be great if we could access it when constructing such objects that would need network access.