Triplea: Eliminate use of Reflection

[RoiEXLab]

This issue is created in order to discusso on how to eliminate the use of reflection on certain places in the game engine code:

GameDataManager.java Class.forName(String).getDeclaredConstructor().newInstance()

This class seems to be responsible for creating delegates from savegames via reflection.
I tried using XMLGameElementMapper instead of this, but unfortunately this class doesn't have mappings for all delegates, so loading a savegame often results in an exception.
The question I'm asking myself now is if I should just add the "missing bindings" to the Delegate map, or if I should create a different map, inheriting from it in order to prevent game xmls to load the currently non-default-loadable delegates.
@DanVanAtta Because you added this class in commit a38df933caa0b29cf0b8a5b2a5e6e61ba6bd583f, did you leave out the other Delegates on purpose, or did you just add the ones that were being used in existing game xmls?

RemoteMethodCall.java Class#forName(String)

I believe this class is responsible for getting reference to remote interface methods in order to use them.
From my quick analysis there is no checking what kind of class reference we even retrieve here, but I believe normally it should be one that is listen somewhere as reference in a RemoteName instance.
Maybe @ssoloff has some insight as he's been refactoring a lot of this stuff recently?
If my assumption is correct, it would probably be a good idea to use a HashMap here as well to avoid the use of reflection.

EndPoint.java Method#*, Class#getMethod

This seems to be the class that actually invokes methods on remote interfaces.
Also in EndPoint#invokeSingle we find the only (indirect) non-test usage of RemoteMethodCall#stringToClass mentioned 1 option further up.
With some extended refactoring we could maybe change both to completely rely on RemoteMethodCalls being mapped to lambdas and therefore eliminate the weird effect of reflection there.
Not sure how this affects compatibility though.

RemoteInterfaceHelper.java Class#getMethods

This class is responsible to sort methods in a remote interface in a specific order and therefore to to assign them a unique index being sent over the network.
Here we could replicate this mapping by creating a map that stores the current index and maps it to a lambda or something so we could easily add new methods to the interface without breaking the compatibility because the index suddenly changed because of that.

MapPropertyWrapper.java Class#getMethods/Class#getDelcaredField

I'm not sure what exactly this class does, but it looks like it's only being used by map creator code in order to create a properties file based on fields of the MapProperties class?
Definitely weird, but should again be easily fixable by creating a simple map and mapping them to properties instead of having some weird field-based logic there.

MapPropertiesMaker.java Class#getMethods

This is a weird class too that seems to export field of the MapProperties class for map making purposes.
It seems to make use of methods starting with "out" in order to print stuff. I'm convinced there is an easier and much simpler way to achieve the same thing.

~GameDataExporter.java Class#getDeclaredField~

~Looks like this class is using reflection to get access to otherwise private fields, even worse is the fact that those fields are hardcodded, so if we ever were to change the variable name, we'd need to adjust the map creator code as well which is bad.~ Closed via #4642

So we're actually kinda close to have a reflection free engine (apart from serialization of course, which is similar regarding compile time checks) and there are a lot of tasks that are easy to implement.
The question is just how we want to approach a couple of things.

[ssoloff]

Thanks for taking the time to create this list! It's nice to have a strategic view of the remaining reflection usage in the code.

---

RemoteMethodCall.java
EndPoint.java
RemoteInterfaceHelper.java

I'm not sure how easy it will be to fix the existing RMI subsystem without breaking compatibility.

One advantage we have in trying to tackle this problem in the network layer over the persistence layer is that we have more control over which clients are active in the wild than we do with save games. What I mean by that is that, while a save game may be "active" for months or a year (I saw someone on the forum not too long ago say they tried to pick up a game they left off with more than two years ago), the network protocol is only active for the duration of a network game session.

Thus, we should be able to gradually transition users to a new network protocol over the course of two or three stable (not necessarily incompatible) releases. During that period, the client would support both the new and old protocols. It would simply try the new protocol first, falling back to the old protocol if the server doesn't respond.

TL;DR: If might be a better ROI to focus on writing a reflection-free RMI replacement that works in parallel with the current system for a few releases than trying to retrofit the current system to remove its use of reflection.

---

GameDataExporter.java

This seems like the lowest-hanging fruit. It looks like we'd just have to provide accessors on the types that are the target of reflection (e.g. GameProperties), right?

[RoiEXLab]

@ssoloff

GameDataExporter.java

This seems like the lowest-hanging fruit. It looks like we'd just have to provide accessors on the types that are the target of reflection (e.g. GameProperties), right?

I guess so.

If might be a better ROI to focus on writing a reflection-free RMI replacement that works in parallel with the current system for a few releases than trying to retrofit the current system to remove its use of reflection.

Good point, however we'd need to agree on a system to make this work consistently.
As already brought up by @DanVanAtta we could use Tyrus for Websocket communication. This could be combined with another HTTP server software in order to allow us using a REST API for the most part.
(Login verification, sending ban requests etc. can easily be done with HTTP communication, implementing a chat system not so good)

[RoiEXLab]

Also when we implement a new system, we really need to ensure we can have TLS encryption.
It's just not state of the art anymore to not have the possibility to encrypt traffic in 2018

[RoiEXLab]

Actually you know what?
I'll play around with "spark", a seemingly lightweight HTTP server framework that supports websocket as well as standard HTTP traffic, and seems to be slightly more maintained.
Also it's similar to ExpressJS, so it will feel familiar to me (I assume)

[RoiEXLab]

I want to note that in order to replace the current netcode we need to create a list of all existing functionality first in order to be able to clearly seperate what needs a two way "TCP" connection i.e. Websocket (like the chat, lobby notifications, game server updates like player count) and client-server-one-way connections i.e. simple HTTP(S) for easy stuff like authentification for the lobby (where we could reduce our RSA logic which encrypts the passwords manuelly and use TLS exclusively) and remote game servers, as well as Toolbox commands and GameServer status updates to the lobby where a new player joined etc.
When I have some time I'll try to compile a list of existing functionality, where I'm probably going to need some help to verify I didn't miss something.

[DanVanAtta]

One question I think that comes up is how to track this task/project? I've generally viewed the reflection code as being in two buckets, save game and network (and really a third bucket, like map creator, but those I think we can fix without too many concerns). I would suggest we carve off save game migration and network migration to two projects. That perhaps leaves this ticket as a good place to identify anything else that remains in terms of reflection.

When looking at how to write up Tyrus, an http server made sense to have. A websocket involves a fully custom protocol, adding control flow for every possible message was complex. I think that part of the discussion though is getting focused on how to build up a new server, which opens a number of additional topics out of scope from reflection migration.

I think the outcome of this dicussion perhaps should be additional issues that are further discussions/plans/tracking of how we'll solve each part of this problem (divide and conquer)

[RoiEXLab]

@DanVanAtta You're completely right.
This issue is primarily thought to compile a list of locations where reflection is still happening (mainly driven by me realizing there's still too much of it going on).
Regarding the net code:
In order to start, we should compile a list of existing functionality and decide what should be HTTP and what should be Websocket.
After that we could start adding *Controllers (keeping potential name collisions with existing controllers in mind) to run a double system which allows us to easily migrate stuff regarding moderator actions etc.
We should create the HTTP server in a way making it return 501 Not implemented whenever something is requested that the server doesn't know, and return the version in a request.
This way a client can easily detect when it's too new or too old for a specific action.
For the Websocket Code we need to figure out another consistent pattern though

[DanVanAtta]

I'm liking where this is heading. For the purpose of adding and migrating features, as long as we know what is going to be http vs having a websocket component, I think we can get started. Notably we will not be migrating everything all at once, so it's worth I think only migrating one chunk at a time and doing so well. The first few will be the most interesting as we will be establishing new patterns in terms of framework, conventions, deployment, and local testing.

For versioning, an interesting topic, worth discussing on its own. The best way I've heard to do this is through an application load balancer that can inspect a version parameter and then route the request to a fleet running that version. The benefit there is you can always drop legacy code when doing updates, and you know when clients have all migrated when you stop receiving requests on clusters that are older. But that is not necessarily mutually exclusive to serving a 501. I think though that is getting a bit ahead of ourselves, and off-topic from reflection.

For removing reflection, the better we identify what and how we will migrate it, the better IMO.

[RoiEXLab]

@ssoloff About the recent delegate Renaming:
If a delegate is not present in XmlGameElementMapper I don't think the class can be renamed without causing map incompatibility.
Not familiar with the exact circumstances, but I do think case 1 needs to be eliminated before.

[RoiEXLab]

About the first reflection case again:
Looks like the delegates being instantiated via reflection are only created based on a class string that is somehow stored inside savegames.
Because that compatibility breaks anyways we should be fine though.
Nevertheless we probably want to create mappings for all the constructors anyways.

[ssoloff]

@RoiEXLab Thanks for pointing this out. :+1:

I agree with your analysis that we're technically okay because we've already broken save game compatibility.

However, my removal of @MapSupport in #4274 wasn't quite correct because of the hidden reflective construction in GameDataManager. Since we're still early in 1.10 development, I think the best thing would be, instead of reverting #4274, to just remove the usage of reflection in GameDataManager as soon as possible. It seems like there should be a simple way to share the XmlGameElementMapper code between GameDataManager and GameParser. Obviously, adding in the missing types, as well as renaming the factory class so it isn't so map-specific.

I'll try to get that done by the end of this weekend.

[RoiEXLab]

@ssoloff We might not actually want to re-use XmlGameElementMapper for this kind of task. This class contains all the mappings that are accessible via xml strings, but the actual amount of delegates is a real superset of those delegates.
In addition the class name is not "user"/mapmaker-controlled, but used exclusively internally.
So we could give delegates a unique identifier to be used inside savegames instead of the class name that are then populated into a map for O(1) access as we're breaking compatibility anyways.

[ssoloff]

@RoiEXLab Yeah, I should have clarified that I was thinking of a two-layer approach. Some class like DelegateFactory that was responsible for creating _all_ delegates based on a unique ID. Then the code in XmlGameElementMapper would delegate to DelegateFactory instead of using the delegate constructors directly (that's way too many uses of the word _delegate_ in one sentence :confused:).

[RoiEXLab]

@ssoloff Sounds good 👍
While you're at it, please consider changing it in a way that TestDelegate and TestAttachment can finally be moved into the test sources ane called by test code only.
Currently the compile-time dependency forces us to keep this classes in the normal source folder.

[RoiEXLab]

Updated the main ticket via #4642

[DanVanAtta]

I moved the list of locations using reflection to the wiki: https://github.com/triplea-game/triplea/wiki/Locations-using-Reflection