Further improvements to server starts #4787
Merged
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
Also don't need to _optionally_ copy the campaign, we always have to.
Rather than modify the topology type selection for arbitrary loaded campaigns in `postInitialize(), we now only set them on the basic campaign createdd in `initialize()`. This avoids the need to access zone renderers during initialization, and makes sure we aren't unnecessarily modifying existing campaigns.
The existing message queue (`addMessage()`/`nextMessage()`) was a thread-unsafe FIFO queue. It looks like the intent was to fairly share the connection between channels, but this was not achieved. It would be a good idea, but for now we just replaced all that with a simple `BlockingQueue<>`. Use of the queue has been simplified. `nextMessage()` is now blocking, so threads do not have to explicit `wait()`/`notify()` to avoid spin locking, nor do they have to check `hasMoreMessages()`. A timeout of 10ms is included just to be extra sure that the receive thread will not block indefinitely. Some flags were updated to be atomic where needed. And in `WebRTCConnection`, we don't worry about the peer connection status anymore, just the data channel status.
For `SocketConnection`, only the receive thread calls `fireDisconnect()`, which it now does whenever it exits. Thje send thread can call `close()` in those cases where it needs to bail. For `WebRTCServer`, we no longer call `MapTool.disconnect()` as that implies a clean disconnection. Instead we only call `stopServer()` which signals an unexpected disconnect.
With that, there is no good reason for AbstractServer to keep track of connections. The remaining functionality is part of `Router.reapClients()`, with connection closing in `MapToolServerConnection`
`AbstractServer` now only keeps track of attached `ServerObserver` and notifies them whenever a connection is established. It is up to the observer (`MapToolServerConnection`) to attach message handlers and perform the handshake. This avoids the need for `HandshakeProvider<>`, and means `Handshake<>` is no longer in `clientserver`.
Since the server already knows who the local player is, avoiding the handshakes makes startup times near-instantaneous. We also connect a server and local client through a `DirectConnection` that is much like any other `Connection` but sends messages through `Queue<>` rather than the network stack. This avoids the need for `NilMapToolConnection` which made this a special case at the application level.
Merge PersonalServer back into MapToolServer Now that `MapToolServer` supports direct local connections, we have no need for a specialized (and not fully functional) `PersonalServer` just to avoid start up costs. Instead we have a `NilServer` in `clientserver` that is the simplest fully functional `Server` implementation that never yields new connections. This also marks the return of `PersonalServerPlayerDatabase`, but unlike before it is actually put to use by the personal server.
Instead of `installClient()` we now have `setUpClient()` which does not take a runnable.
Responsibilities were weird (e.g., `MapToolServerConnection.playerMap`), and after cleaning that up `MapToolServerConnection` was left with too little to warrant its existence. Now `MapToolServer` directly contains the `Server`, `Router`, and `MessageHandler`, tying them together via handshakes.
It should not be called for all personal starts because the conclusion of the "handshake" will involve setting it through a `SetCampaignMsg`. This has been fixed so that we only directly call it for the initial personal server with the default campaign so that the user is not staring at a blank screen.
For large campaigns, a local client can wait quite a while for the server to serialize and send the campaign, despite the client already having the same campaign. So now we skip that, simply invoking `setCampaign()` with the client's copy of the campaign. As an added perk, this avoids the client jumping to a random map when a server is started.
… the server, no one else does
Includes UPnP set up, server registration, and the LAN announcer.
Now just like `MapToolClient`, we can query the state and can ensure that we only transition to each state once.
This matters because a `null` server is presumed to indicate a remote connection. In reality, it could mean an unexpected situation occurred for a local server, even a personal server. This changes allows a new server to be started in these cases, avoiding some possibilities for the user to be "soft locked" after an error. We now explicitly null out the server field when connecting to a remote server.
If the client object has been closed, then the disconnect was expected. An unexpected disconnect is one that occurs when the state is `New`, `Started`, or `Connected`.
No functional change, but avoids some redundant clean up and associated logs.
kwvanderlinde
added
refactor
cwisniew
approved these changes
May 22, 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
Improves on #4519, #4542, #4654 & PR #4771.
Fixes #2487 "by accident" (just the view switch bug, not the related features)
Description of the Change
This goes further to clean up personal server starts in a way that also benefits hosted servers. There are changes to clientserver and the main project.
clientserver
Some threading issues with connections were found during testing and have been fixed. This includes using a
BlockingQueue<>instead of our custom channel-based FIFO, using atomic shared flags, and clearing up responsibilities for notifying on disconnects.But the main change is to reduce the responsibilities of
Serverimplementations. The connection maps have been moved into a separate componentRouterthat is able to send messages to connections. The message handler and handhsakes now live entirely in application code (MapToolServerConnection) so the server does not have to know about these. This leavesServerthe sole responsibility of listening for incoming connections and informing theServerObserverwhen a connection is established.There is also a new
NilServerandDirectConnectionthat are fully-fledged - but simple -ServerandConnectionimplementations. Their use is described below.Main project
The purpose of the above change is to enable some more nuanced logic in the application code. Now that the application code controls handshakes, it can skip them for local connections (a server already knows and can trust the local client connecting to it). We also don't need to serialize and transmit the campaign to the local client because it already has a copy of the campaign. A nice benefit to the last point is that starting a server no longer switches the user to a random map.
Since we can do all that at connection time, we don't need
PersonalServerorNilMapToolConnectionto provide ongoing special logic for local connections. This can all be done in or through the standardMapToolServerandMapToolConnection. For local connections (personal servers, local hosted servers), we useDirectConnectionbetween the client and server instead of aSocketConnectionorWebRTCConnection, and we install this connection directly to theRouter(no need to somehow get it throughServer). And for personal servers, we use aNilServerbecause we don't need to listen for or accept any connections.With all the above done, personal server starts are essentially instant since they have almost nothing to do. Hosted server starts take advantage the same efficiencies and are also quite quick now, with only server registration and such being a possible source of delay.
Possible Drawbacks
Nothing functional. The discrepancy between local and remote connections at connection-time could be seen as a downside when navigating the source code.
Documentation Notes
N/A
Release Notes
This change is