WIP connection manager

[fraillt]

This PR provides a concept of a connection with connected/disconnected states over UDP.
It provides two traits ConnectionManager and SocketManager, and implementation of these can be provided by the user. More discussion about it in this issue #242.
The emphasis of this PR is a connection manager, although it also provides a socket manager concepts, after talking to @TimonPost on discord, I think it is wise to split it apart.
Currently, a socket manager basically does two things:

• tracks system errors and bytes sent/received/ignored
• based on this information, it decided if a connection can be created, and when to destroy it.

Probably it is wise to implement some metrics tracking in laminar itself, (some discussion about it is here #153.) and change SocketManager to ConnectionManagerFactory which is only responsible for creating ConnectionManager.
Decision making of whether to accept or reject new address can be addressed in the future.

[TimonPost]

Great work @fraillt !!

round 1

[TimonPost]

I would like to see a short description of what a socket manager is

[fraillt]

Would you agree that I split SocketManager into two parts, as I wrote in the comment?
One would be ConnectionManagerFactory two methods accept_remote_connection, accept_local_connection, and the other would be SystemTracker?, SocketTracker?, MetricsCollector? ... you name it :) that would basically have these track_ methods.

[fraillt]

I extracted part of VirtualConnection into separate class ReliabilitySystem which knows how to parse from bytes to packets, and vice- versa. And result type implements Iterator trait, which is convenient.

I want to finish extract stuff from VirtualConnection, and leave it only simple struct with no implementation with these fields:

pub struct VirtualConnection {
    pub remote_address: SocketAddr,
    pub reliability_system: ReliabilitySystem,
    pub connection_manager: Box<dyn ConnectionManager>,
    pub current_state: ConnectionState,
}

and I would really like to put last_heard and last_sent into separate struct named ActivitySystem or something, and put it in connection manager.
Do you approve these changes?

[TimonPost]

It is difficult to say if I would approve it. I do have questions and concerns. 1) This PR is quite big a lot of tests and logic is bound to virtual connection. So breaking that will break other stuff. That's where my second question comes in 2) What is your motivation behind those changes? Is it needed, are you having problems with implementing the feature. I prefer not to change anymore other than that is needed for the connection system. If you see some nice abstraction for VirtualConnection I would be glad to have it in another PR, because that's where I left off, and couldn't find a perfect solution for moving all the logic without creating millions of abstractions.

[fraillt]

Ye, I do agree with you, it would be best if I freeze this branch and only fix bugs and write tests. I'll remove this last commit.
But I do see this as an improvement, because of Rusts borrowing rules, this would allow for better optimizations in the future. As a side effect it is easier to understand for developer as well:) basically what I don't like at current state is that I have ConnectionManager and ConnectionState in VirtualConnection as public fields, that is not used in the class itself, but used by higher abstractions, I put them here because it was the only place to store per connection information:) My extracted new interface is basically renamed VirtualConnection:)
But lets do cleanup things latter, as separate PR

[jstnlef]

I've taken a bit of time to look through this and while I appreciate the need for connection management in laminar, I'm not too keen on this implementation. This feels very "bolted" on and not at all integrated with the solution proper. For example, I would expect something called ConnectionManager to own (and or manage) the connections. This makes the interaction point with it in recv_from extremely awkward.

let conn = self.connections.get_or_insert_connection(
    address,
    &self.config,
    time,
    manager,
);
ActiveConnections::update_connection_manager(
    conn,
    &self.event_sender,
    self.manager.as_mut(),
    &mut self.socket,
    time,
    self.tmp_buffer.as_mut(),
);

Another bit of weirdness is around the caller API. Having to wrap socket.get_event_sender() as SocketEventSender(socket.get_event_sender()) is not exactly ergonomic nor obvious to users.

I'm also worried about the proliferation of the Either type as well as the increase in the size of wrapped types. e.g.
Err(ErrorKind::SendError(SendError(Either::Left(error.0))))
and
Option<Result<Either<GenericPacket<'a>, ConnectionState>, ConnectionManagerError>>

Anyway, I urge you to rethink your implementation. I really do think that this feature is necessary and want to see it be a success because I'm really digging the end result but I think that it might be worth taking some extra time to figure out how to integrate it more holistically.

Top suggestions:

1. Have the new ConnectionManager own the lifecycle of connections. Creation, updates, deletion, etc of VirtualConnection
2. Replace usages of Either type with enums of an appropriate name. If you can't think of a name that unifies the 2 types, perhaps you're dealing with 2 different behaviors that should be handled by different functions/methods.
3. Let's see if we can get away with fewer wrappers. Looking at you SocketEventSender and ConnectionEvent. That said, the split of SocketEvent into ReceiveEvent and SendEvent is good.
4. The bouncing between socket methods and SocketManager methods is strange. I'm not even entirely sure SocketManager is needed since it isn't exactly "managing" sockets. I would say we should toss it.

[fraillt]

I'm glad you addressed these points because most of them I don't like either. But they are more or less side effects of previous discussions. I guess I was trying to address too many things in this PR.

This code, that you quoted, looks weird for me too, partially because part of the connection is constructed inside ActiveConnections and part of it in Socket implementation.
As I have mentioned earlier, I think we need to split VirtualConnection into more parts, because it is too broad term now.

SocketEventSender is, not well thought enough, product of discussion #242, the concern was that client.send(ConnectionEvent(addr,SendEvent::Packet(Packet::unreliable(addr, .......)))); is too verbose, compared to client.send(Packet::unreliable(addr, .......)); and I fully agree.
Maybe instead of providing crossbeam Sender directly through socket get_event_sender method, we should return a wrapper object, with only connect, disconnect, send_packet methods?

SocketManager is also product of discussion in the #242, there was a lot discussion about how DoS should be handled, and socket manager is an attempt to provide interface, that could be used to handle connection lifetime, but I do agree this is not necessary and I already expressed my concern in initial comment of this PR that maybe it should be changed to ConnectionManagerFactory?

I would expect something called ConnectionManager to own (and or manage) the connections.

Maybe you have a better name to suggest?

• ConnectionStateManager - not precise enough, because it does more than one thing?
• ConnectionPostPreProcessor + ConnectionStateManager ?
• ConnectionHandler?

Last questions: why do you think ConnectionEvent is bad abstraction? I think it perfectly unites two heterogeneous things: what event happened, and who is the target of event. With this separation in place, events could also be reused in the application layer, where you usually have a separate object for each address already.

Ok, so until these questions are answered, I'll do the following:

1. Change a return type of update method in ConnectionManager:

pub enum ConnectionManagerEvent<'a> {
    NewPacket(GenericPacket<'a>),
    NewState(ConnectionState),
    Error(ConnectionManagerError)
}

2. Rename VirtualConnection to ReliabilitySystem, and new VirtualConnection would look like this:

pub struct VirtualConnection {
    remote_address: SocketAddr,
    reliability_system: ReliabilitySystem,
    connection_manager: Box<dyn ConnectionManager>,
    current_state: ConnectionState,
}

3. Replace Either with more precise names or functions.
4. Leave ConnectionEvent as it is, but remove entirely SocketEventSender, and wait for your suggestions, because I don't know what is the best approach to address these.
5. Split SocketManager into two parts, ConnectionManagerFactory and MetricsCollector as I wrote in here.

So, let's get to work :)

[fraillt]

This PR is getting bigger, and bigger ...
I want to point out, that MetricsCollector is unrelated to connection manager, and currently basically serves as a logger. I can remove it if you want, but this might be useful in future for #153 and #239.

Code could be made much cleaner if we simply ignore SendError from the crossbeam.
As I understand it can never happen anyway because Socket always stores sender and receiver pairs.

[fraillt]

Since this PR gone out of control, in terms of what amount of code has changed. I decided to split it in smaller PR and prepare a code base for easier integration of connection manager in the future.