Refactor client-side and server-side state, split player database responsibilities, and enforce better lifecylce for servers and clients #4771
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This one doesn't actually set up a server, so there's no point keeping it around. A real standalone server can always be added in the future.
The socket streams are now accesed only in their respective threads. This avoids possible `IOException` being thrown by the `SocketConnection(String, Socket)` constructor. If there is an error getting either of these streams, they will be reported like any other `IOException` encountered while the connection is open.
In other words, `Handshake.startHandshake()` no longer throw `ExecutionException` or `InterruptedException`. This greatly reduces the number of exceptions we need to account for.
The new `MapToolClient` brings together several things that used to be part of the global static state in `MapTool`. These include: - The connection itself - The local player - The client-side message handler - The current campaign - The server command object - The disconnect handler Aside from organizational benefits, this has the perk that each of these component is directly associated with the connection, avoiding any possibility that they might affect another connection depending on exact timing of events. It also means we can rely on some of the things always being present while before it depended on just how far along we were with app initialization. E.g., `MapTool.serverCommand()` never returns `null` anymore.
One test was relying on the basic campaign (`CampaignFactory.createBasicCampaign()`) being set in the app state, as it needed properties from this basic campaign. Now that we actually set an empty campaign by default, this test broke. It was fixed by simply setting the campaign during test setup instead of implicitly relying on it being in the expected state already. One test for lenient JSON parsing was attempting to test fallback behaviour for the parser. In order to trigger the fallback, it was relying on a `NullPointerException` due to a local player not having been set up since app initialization was not done. But now that we always have a player available through `MapToolClient`, this exception is not thrown and the test fails. The data for the test is also not valid for the test case, and if used in real MapTool token properties it would not have the asserted result. So this data has been replaced by data that is expected to produce the lenient JSON result. Another few tests relied on special logic in `MapToolVariableResolver` that allows setting token properties in tests without going through a `ServerCommand` as that requires frame state and others to be set up. It does this by checking whether `MapTool.getServerCommand() == null`, a case which no longer holds. As a result, `NullPointerException`s were being thrown where they weren't before. This was fixed by changing the tests to explicitly use a variable resolver that writes directly to tokens and never goes through `ServerCommand`. In the future this could be mitigated by having different `ServerCommand` depending on the situation, or by reducing the amount of dependence our model code has on the frame.
These were removed due to not being used: - `PlayerDatabaseInfo` along with its accessors - `PlayerDatabase.recordsOnlyConnectionPlayer()` - The checked exceptions of `PlayerDatabase.getOnlinePlayers()` and `PlayerDatabase.getAllPlayers()`
All servers now implement `IMapToolServer` with any generic code using that. Similarly, connections to the server are represented by `IMapToolConnection`, which might be a `NilMapToolConnection` that is for use with personal servers. `ServerHandshake` has been updated to hold onto a `MapToolServer` instance rather than looking it up all the time. This avoids any possibility of a server handshake picking up the wrong server in cases where the handshake is slow, and a server restart happens in the meantime. There is no more concept of a "personal server config". Instead a personal server is a trivial concept of a server that doesn't require a socket configuration or player passwords or anything since it is not a real server that can be connected to.
The main change here is that `PlayerDatabaseFactory` no longer carries state about the current player database, it just provides instances of `PlayerDatabase`. The current database is maintained in `MapToolClient` instead. There are a lot of change listeners that aren't needed anymore as a result and the whole flow of player databases is easier to follow. Surprisingly, `PersonalServerPlayerDatabase` was essentially unused and has been removed. Although it was set when starting a personal server, it would be promptly replaced by a `LocalPlayerDatabase` once the handshake completed. During that short time it didn't do anything special, so we just use `LocalPlayerDatabase` now. Some methods in `PlayerDatabase` were specific to server-side use. These now live in the `ServerSidePlayerDatabas` interface. This simplifies `LocalPlayerDatabase` which is only used client-side. The `Players` constructor now takes a `PlayerDatabase` instead of having to look it up for each operation.
`MapTool.createConnection()` and `createLocalConnection()` are very similar and have been consolidated by introducing a new `installClient()` method. The ordering of `server.start()`, `client.start()`, and `installClient()` is now more definite. When starting a server, the ordering is always: 1. Create and set the server 2. Create and set the client 3. Start the server 4. Start the client Part of that is a change where we always connect immediately to a local server. This avoids any possibiliy that we momentarily hang onto an old client after starting a new server.
The logic is essentially identical in each - we just need to know where to get the local player, player database, server policy and a connection.
The methods `MapTool.startServer()`, `MapTool.stopServer()`, and `MapTool.disconnect()` have been detangled to work better together: - `startServer()` was already good as it just starts a server. But now it also takes care of UPnP setup, which for some reason was done by the caller before. - `disconnect()` is called whenever connection state needs to be cleaned up, including UI adjustments. It no longer does anything related to server teardown, e.g., no more does it close upnp ports or stop the announcer. - `stopServer()` now only tears down server state. Everything done in `startServer()` is undone in `stopServer`. `stopServer()` no longer calls `disconnect()`. During expected disconnects, we still expect `disconnect()` to be called, but also require `stopServer()` to be called in the case of a local server. As for unexpected disconnects, connection state is still cleaned up by our disconnect handler. However, the disconnect handler no longer has to clean up for expected disconnects and also correctly handles the case of a local server unexpectedly disconnecting. This all avoids the redundant "You have disconnected" messages, and avoids the bad state that sometimes seen when a server unexpectedly goes away.
…rgument for optional server command
Callers can no longer directly modify the result of `MapToolClient.getServerPolicy()`, but must instead write it back. The good news is we always technically did this, so that wasn't a big ask. To support this, `ServerPolicy` now has a copy constructor. The methods on `MapTool` for updating the server policy have been removed in favour of the methods on `MapToolClient` and `ServerCommand`.
Where possible, `ClientHandshake` and `ClientMessageHandler` now use their associated `MapToolClient` rather to reaching out to the global `MapTool` state.
Much like PlayerDatabase, the connected player list is associated with a specific client-side connection and therefore belongs in `MapToolClient`.
kwvanderlinde
force-pushed
the
bugfix/4519-4542-4654-server-init-and-shutdown
branch
from
7c636c1
to
392d148
Compare
|
Just wanted to review this, but still draft. :) |
Undrafted now 🙂
I am generally in favour of what I'm seeing on your branch, but would like to keep this PR more focused (it's already pretty large). I actually have a lot of other potential changes written up for Maybe we can talk about this further somewhere else (#3790 I guess), but I also have some ideas on how servers can be reworked that in some ways lines up with your branch there, but in other ways is different. The main thing in my mind is that it should be trivial for a client to connect to its own local server, regardless of whatever connection mechanism remote clients have to use. A separate concern to that is whether the server starts up successfully so that remote clients can actually connect. |
cwisniew
approved these changes
May 7, 2024
I agree with @kwvanderlinde lets do it in a separate PR |
This was referenced May 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Identify the Bug or Feature request
Address #4519 , #4542, #4654.
Probably also addresses #3756 but was having a hard time reproducing that exactly.
Description of the Change
This changes is primarilty a refactoring that better separates client and server concepts and enforces better lifecylces of both. But it touches on a few separate parts of the codebase, so I'll cover them separately below.
Client-side changes
Mainly this pulls various parts of the
MapToolglobal state into a newMapToolClientclass. This class represents the client-side of a connection, analogous toMapToolServerfor the server-side. This new class keeps track of the connection itself, the local player, the current player database, the current campaign, etc. This allows us to clearly associate each one and set up the necessary dependencies between them, rather than having each look up the current global state and hope that whatever they find was created for the same connection.Server-side changes
This reworks personal servers to avoid most possibilities for failure and to generally simplify the case of a personal server. Before, we would run a real server on a random port with its own copy of the campaign, and would have to keep that campaign in sync with the client by sending it network messages. Now we have a trivial
PersonalServerclass that does not need its own campaign copy, and therefore does not need to respond to any messages. This avoids several potential failure states, and makes personal server creation an instant process, which in turn avoids timing issues that can occur when loading large campaigns.Player database
At the intersection of the above changes was the
PlayerDatabase, and more importantly, thePlayerDatabaseFactory. Before, we kept track of the current database inPlayerDatabaseFactoryitself. NowMapToolClientkeeps its ownPlayerDatabase, andMapToolServerkeeps its ownPlayerDatabase(for local servers they are the same database). This meansPlayerDatabaseFactoryis now just a factory and not a vehicle for application state, which in turn allowed us to cut out a lot of code dedicated to notifying and keeping track of the databases.PlayerDatabaseitself was reworked a bit as well. Some methods were specific to server-side databases, so those have been moved into their ownServerSidePlayerDatabase. This simplifiesLocalPlayerDatabase(which is purely client-side) and means we don't have to try and interpret some of these server-side methods in a client-side way.Lifecylce improvements
With things a bit more separated, we have a clearer process for starting servers, connecting to servers, and disconnecting.
When starting a server, we now create the server (personal or other local server), create a corresponding client to connect to it, then start the server and then set the client. When connecting to a remote server, we null out the local server and create a client to connect to the remote serve, then start it. Doing it this way means the client and the server will be set together, rather than having periods of time where one or the other isn't set, or might be set to old state.
On the disconnecting side, we have altered the relationship between
MapTool.stopServer()andMapTool.disconnect():disconnect()only deals with cleaning up client-side connection state, including adjustments to the UI as needed. It no longer does any server teardown such as UPnP port closure or accouncer shutdown.stopServer()is where server state is cleaned up (if a local server is running). It is a mirror imge ofstartServer(), cleaning up everything thatstartServer()set up.stopServer()no longer cleans up connection state throughdisconnect().This better allows us to handle both expected and unexpected disconnects, and avoids the redundant "You have disconnected." messages that show up when disconnecting for a server.
Possible Drawbacks
Documentation Notes
Release Notes
This change is