Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Further improvements to server starts #4787

Merged

Conversation

kwvanderlinde
Copy link
Collaborator

@kwvanderlinde kwvanderlinde commented May 21, 2024

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 Server implementations. The connection maps have been moved into a separate component Router that 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 leaves Server the sole responsibility of listening for incoming connections and informing the ServerObserver when a connection is established.

There is also a new NilServer and DirectConnection that are fully-fledged - but simple - Server and Connection implementations. 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 PersonalServer or NilMapToolConnection to provide ongoing special logic for local connections. This can all be done in or through the standard MapToolServer and MapToolConnection. For local connections (personal servers, local hosted servers), we use DirectConnection between the client and server instead of a SocketConnection or WebRTCConnection, and we install this connection directly to the Router (no need to somehow get it through Server). And for personal servers, we use a NilServer because 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

  • Fixed a bug where starting a server would switch to a different map.

This change is Reviewable

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.
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 kwvanderlinde added refactor Refactoring the code for optimal awesomeness. performance A performance or quality of life improvement labels May 21, 2024
@kwvanderlinde kwvanderlinde self-assigned this May 21, 2024
@kwvanderlinde kwvanderlinde marked this pull request as ready for review May 21, 2024 22:31
@cwisniew cwisniew added this pull request to the merge queue May 22, 2024
Merged via the queue into RPTools:develop with commit 3951887 May 22, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance A performance or quality of life improvement refactor Refactoring the code for optimal awesomeness.
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

Random view switch when starting and stopping server.
2 participants