Added ability to create multiple servers, including a Central server by kbarkevich · Pull Request #96 · sepalani/MH3SP

kbarkevich · 2023-03-23T01:33:20Z

This PR implements cross-server communication under the following structure:

The Central server (server 0) is responsible for accepting first-stage connections from clients who are just beginning to enter City Mode. This server runs all 4 server types: FMP, OPN, RFP, and LMP. This server does NOT itself host any in-game players, instead displaying a Server List to the players that is representative of the connected Remote servers. Players who select a server from the Server List are passed the connection info of the new server, and then disconnect from the Central server.

The Remote servers (servers 1-...) are responsible for maintaining the connections of in-game players. Each Remote server only consists of a single FMP server. Each Remote server is represented in the Server List by a name designated in the config.ini file (Valor 1, Valor 2, Greed 1, etc.) and hosts a number of Gates/Cities. Upon starting, each Remote server forges a connection to the Central server which is maintained for the duration of the server's existence. This connection begins with a handshake between the Central and the Remote server, during which the Remote server informs the Central server of its identifying information (its name, its ID, its list of gates/cities, the port to which players should connect, etc.) The Remote server will then appear in the Central server's Server List.

The Central server and the Remote servers periodically update each other on their information, with the Central server acting as the intermediary. On an interval, each Remote server will send the Central server its most recent information on the Gates, Cities, Circles, and Players (Sessions) contained within it. On another interval, the Central server will send each Remote server a "status update" on each of the other Remote servers based on its most recently-acquired information from each one. This is so that every Remote server has enough information on the other Servers to facilitate things like Player Searches, City Searches, Friends List online statuses, Warping, and transferring from one Remote server to another Remote server.

The config file config.ini was modified to contain the necessary information for the starting of the Central and Remote servers, and their connections to each other:

Under the [CENTRAL] category are CentralIP, which should contain the IP address of the Central server, and CentralCrossconnectPort, which should contain the port that the Central server will open to the Remote servers for inter-server communication. This category should be present in the config.ini files of each Remote server as well as the Central server, so that the Remote servers know how to contact the Central server.

Under each of the [SERVER] categories are Name, which should contain the plaintext name of the server that will be displayed in the console, log file, and Server List, ServerType, which should contain a number from 1-4 corresponding to the category the server should fall under in the Server List (1: Open, 2: Rookie, 3: Expert, 4: Recruiting), IP, which should contain the IP address of the server, and Port, which should contain the port that the Remote server will open up to connecting Players. Only the [SERVER>number>] category for the Remote server that is being launched is required to be present; no Remote server will ever access the other Remote servers' information through the config file.

The type of server that is launched corresponds to the new launch parameter -S, or --server_id, which accepts an integer. To start the Central server, this launch parameter should be left at its default or set to 0. To launch a Remote server, this launch parameter should be an integer >=1 that corresponds to a category in the config file config.ini. For example, --server_id==1 would launch a Remote server with the information contained under the config file's [SERVER1] category.

sepalani

Here is a cursory review. In its current state, this draft doesn't meet the code quality we're seeking for the project:

It doesn't seem scalable
Has some drawbacks including security issues (ex: network link isn't secure/SSL)
Lack i18n support and current design might prevent it
Many PEP8 violations (ex: line length)
Some functions are way too big and not appropriately documented/justified
It lacks self-documentation/clarity
The code isn't split into files when needed
Shadowing exception with try-except
etc.

These are points that I already mentioned in my previous reviews (#77 (comment), #58 (review)).

sepalani · 2023-03-23T07:57:34Z

config.ini

+[CENTRAL]
+CentralIP = 0.0.0.0
+CentralCrossconnectPort = 8300
+
+[SERVER1]
+Name = Valor1
+ServerType = 1
+IP = 0.0.0.0
+Port = 8301
+
+[SERVER2]
+Name = Greed1
+ServerType = 4
+IP = 0.0.0.0
+Port = 8302
+


This approach doesn't seem right to me:

It feels cumbersome and not scalable

Won't take into account i18n

Might clash with FMP server config

Use ports not reserved by Capcom for the game (past websites suggested TCP ports 8200~8299)

I've switched the ports used in the default config.ini to be within that suggested range.

sepalani · 2023-03-23T08:52:21Z

mh/pat.py

+        #server = self.session.join_server(index)
        config = get_config("FMP")
-        fmp_addr = get_ip(config["IP"])
-        fmp_port = config["Port"]
+        central_fmp_addr = get_ip(config["IP"])
+        central_fmp_port = config["Port"]


This should be moved closer to where its use (i.e. inside the if/else statement). The comment should be removed or documented.

This has been fixed.

sepalani · 2023-03-23T08:56:25Z

mh/session.py

        self.hunter_info = pati.HunterSettings()

-    def get(self, connection_data):
+    def serialize(self):


The way (de)serialisation currently looks seems overkill. It doesn't respect the current coding style (PEP8) with the lines being way too long. Annotation and metaclasses should be used to make it cleaner.

mh/session.py

sepalani · 2023-03-23T09:18:02Z

mh/state.py

+class PacketTypes:
+    FriendlyHello = 0x0001
+    ReqConnectionInfo = 0x0002
+    SendConnectionInfo = 0x0003
+    ReqServerRefresh = 0x0004
+    SessionInfo = 0x0005


This deserve to be in its own file, it's not a "state" anymore, you're creating server/client classes.

Moved to other/cache.py

sepalani · 2023-03-23T09:19:29Z

mh/state.py

+    SessionInfo = 0x0005
+
+
+class CentralConnectionHandler(object):


Ditto, it has to be in its own file. Do you have a solid reason not to inherit from an existing python class?

Moved to other/cache.py

mh/state.py

mh/pat.py

mh/state.py

mh/pat_item.py

InusualZ

Overall, I like the design of this. The only part that I don't quite like is the serialization/deserialization, but it's not a deal breaker for me.

You should avoid waiting for something to happen in a loop. Instead use blocking function call. That way you don't waste cpu cycles waiting for something and let the os wake up your thread when conditions are met.

I don't think the cache.py file belongs to the mh folder. Since it's only for inter-server connection maybe a more appropriate folder would be something like inter.

This is my first pass onto this branch, one of many. One thing missing that we discussed is a ping system between the external fmp server and the central fmp server.

mh/pat_item.py

mh/session.py

other/cache.py

InusualZ · 2023-03-26T23:31:25Z

other/cache.py

+                self.get_handler(outbound_session[0]).SendSessionInfo(outbound_session[1])
+
+    def maintain_remote_server(self, refresh_timer):
+        while True:


Like said on discord, we can't have a while True: because this is run in a thread and the python interpreter wouldn't exit the process until all thread are finish/joined. So you must replace this True with a variable that we can control.

This has been fixed. Also added a ServerShutdown packet to ensure the Central server is notified instantly when a remote server is shut down.

other/cache.py

other/utils.py

fixup! State fixup! session STATE fixup! state Added ability to create multiple servers, including a central server Basic cross-server communication the work continues Basic central->remote server scheme functional (no server changing yet) Fixed bug that prevented cross-remote server travel Fixed bug where players could not log off Added central players cache to regulate in-use capcom ids for logging-in players, even when the owner of the capcom id is no longer connected to the central server Removed debug comments Added player names to database Removed debug comments Added SSL certificate verification Fixed bug preventing Remote servers from going offline without crashing the Central server Added updating servers version so players are always displayed with the latest server information in the server browser. Split cache.maintain_connection() into smaller methods Removed conflation of "index" and "server_id". All indexes are now converted to server_ids to support "gaps" in the server_ids in case a server goes offline. Instantly inform all Remote servers when one of them disconnects, to minimize the chances of clients being sent a list including a now-nonexistent server. Moved cache-related code to its own module Addressed Sepalani comments Addressed more sepalani comments Corrected default config.ini TCP port usage to be within suggested range Addressed InusualZ comments Addressed InusualZ comments fixup! session DB fixup! database Pep8 compliance Changed user switching method away from a while loop Remote server capacity can vary Added ability to toggle Crossconnect SSL off via the config file Added debug messages Added long-term database support (via a local file) to the multiprocessing system. Fixed cross-server friends list implementation Implemented basic compression of empty objects to decrease network traffic Created proper reconnection methodology for cross-server interruptions and cleaned up giant blocks of code Added graceful client notification for if there are no Remote servers available ("server maintenance") Fixed empty capcom ids list bug Cache: Type hints as comments This also fixes a few bugs catched because of the annotations Cache: Rework connection to remote central server Cache: `maintain_central_server` to `server_cache` [Fixup] Dangling Connection Fixed timeout issue by adding explicit no_timeout mode Fixed hanging client bug fixed bug with connection order Added rapid Remote-to-Central player disconnect notification Improved text for the "Capcom ID in use" error. Add client identifiable logging messages Properly handle when connections fail to be accepted

sepalani requested changes Mar 23, 2023

View reviewed changes

kbarkevich force-pushed the multi-server branch 2 times, most recently from f542c4e to da4f41c Compare March 25, 2023 17:28

InusualZ requested changes Mar 25, 2023

View reviewed changes

mh/pat.py Outdated Show resolved Hide resolved

mh/pat.py Outdated Show resolved Hide resolved

mh/state.py Show resolved Hide resolved

mh/pat_item.py Outdated Show resolved Hide resolved

InusualZ requested changes Mar 27, 2023

View reviewed changes

kbarkevich force-pushed the multi-server branch from b7848b4 to eded66c Compare March 27, 2023 03:08

InusualZ force-pushed the multi-server branch 5 times, most recently from 9315c3e to 0de4000 Compare June 20, 2023 14:43

InusualZ force-pushed the multi-server branch from a7b3ddf to df6987a Compare June 29, 2023 22:10

kbarkevich force-pushed the multi-server branch 2 times, most recently from ccaf8a5 to d932cea Compare July 1, 2023 02:13

InusualZ force-pushed the multi-server branch 2 times, most recently from de2608d to a5aeb62 Compare July 15, 2023 15:32

kbarkevich force-pushed the multi-server branch 2 times, most recently from b36a418 to 2c52d75 Compare September 15, 2023 19:31

kbarkevich force-pushed the multi-server branch from 2c52d75 to f7e90bd Compare September 16, 2023 17:50

Conversation

kbarkevich commented Mar 23, 2023

Uh oh!

sepalani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

InusualZ left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

InusualZ left a comment •

edited

Loading