Separate ServerConnection and ClientConnection #4078
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
Both `SocketClientConnection` and `WebRTCClientConnection` create a message-sending thread and pass a reference to themselves along. But the thread classes are non-static inner classes, so they already implicitly have such a reference. More importantly, some `Connection`/`AbstractConnection` methods are only public so that these threads can access them via the explicit reference, whereas the implicit reference would allow for protected methods to be used instead.
Akin to `AbstractServerConnection`, we now have `AbstractClientConnection` which can provide common functionality that is specific to `ClientConnection` implementations. The status quo is that such functionality lives in `AbstractConnection`, thus providing `ServerConnection` functionality it does not need. `AbstractClientConnection` will be filled in later.
The `Connection` methods `addMessage()`, `hasMoreMessages()` and `nextMessages()` were only used internally for `ClientConnection `implementations, through and supported by `AbstractConnection`. The methods are no longer publicly exposed, and the implementations have been moved out of `AbstractConnection` into `AbstractClientConnection`
`AbstractConnection` provided utility methods for reading messages from a stream, decompressing them, and dispatching to handlers. This has all been moved into `AbstractClientConnection` instead.
`DiconnectHandler` used to deal in `AbstractConnection` despite only ever being used with `ClientConnection` objects. Additionally, only `ClientConnection` can fire disconnection events, so the methods for registering `DisconnectHandler` has been moved from `Connection` to `ClientConnection`. `Connection.fireDisconnect()` is actually an internal utility method and is no longer exposed publicly.
Only `ClientConnection` is able to notify `ActivityListener` since only `ClientConnection` is responsible for reading and writing messages. The registration methods have been moved from `Connection` to `ClientConnection`, with the `notifyListeners()` helper method being also moved from `AbstractConnection` to `AbstractClientConnection`.
`ServerConnection` was only ever used with one handler (`ServerMessageHandler`), which was set immediately after the `ServerConnection` was created, and would not change through the lifetime of the `ServerConnection`. Additionally, `AbstractServerConnection`'s nature as a `MessageHandler` was only for piping messages from `ClientConnection` into the `ServerConnection`'s own dispatching mechanism. So instead of relying on dispatching in `AbstractServerConnection`, we now require a `MessageHandler` to be passed at the time of creation, and we attach it to each `ClientConnection` as they are created. This means only `ClientConnection` need to know about message dispatching. So the registration methods could be moved out of `Connection` into `ClientConnection`, and the dispatching implementation out of `AbstractConnection` into `AbstractClientConnection`.
Everything of use has been removed from `AbstractConnection`, so there is no need to keep this as a common base for `AbstractServerConnection` and `AbstractClientConnection`.
Now `ServerConnection` and `ClientConnection` have their own `open()` and `close()` methods. In the case of `ServerConnection`, the method was renamed to `start()` to indicate its purpose more clearly, as it is not related at all to the opening of connections as in `ClientConnection`. Both `ServerConnection` and `ClientConnection` now extend `AutoCloseable`, as their `close()` methods have the same expectations.
The `Connection` interface has already had everything removed from it, and it is not directly used by callers anymore. As it no longer provides any value, it has been removed.
The `sendMessage()` overloads in `ClientConnection` and `ServerConnection` allow the channel to be omitted. Implementations simply delegate to the other overload by providing a `null` channel. As this is the required behaviour, this is better done in the interface via a `default` method.
This type does not represent a connection, but instead represents a server capable of connecting to any number of clients. The new name reflects this fact.
Also the package name has been changed from `client` to `connection`. This type represents a connection between a server and a client. It is to be used both server-side and client-side, and so the use of the term "client" only distracts from its purpose. Similarly, the package name would suggest some client-side utilities, but in reality contains implementations for connections.
cwisniew
approved these changes
May 21, 2023
cwisniew
added
the
code-maintenance
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
Implements #4076
Description of the Change
The change basically follows the plan set in #4076, moving methods and fields around according to the actual responsibilities for
ServerConnectionandClientConnection. The goal was to leave the usage as-is, only adjusting names and the location of methods and fields to align with expectations and to make theclientserverpackage clearer. The commits detail exactly what moved where, and it may be easier to see the changes toclientserverin the commit diffs rather than the full PR diff.There was one usage change, which was to remove message dispatching from
ServerConnection. It was only ever used with a singleServerMessageHandleranyways, which was set immediately after the creation of theServerConnection. Now we instead require the handler to be passed as a constructor parameter, and then register it onClientConnectiondirectly. This is simpler than having theClientConnectiondispatch toServerConnectionand thenServerConnectiondispatch to theServerMesageHandler. Removing the dispatching also enabled a full separation ofServerConnectionandClientConnectionwithout requiring more drastic refactorings.After the rename, we're left with these types and responsibilities:
Connection(used to beClientConnection) decorates and opens connections, adding message sending and receiving capabilities.Server(used to beServerConnection) waits for clients to connect, createsConnectionobjects to represent them, and broadcasts messages to selected clients over the correspondingConnection.Possible Drawbacks
Should be none, unless I messed up and change behaviour during one of the steps.
Documentation Notes
N/A
Release Notes
This change is