Codebase improvements.

[fraillt]

This PR doesn't add or remove any existing functionality or tests, it only solves a few issues in a codebase.

1. Better interface separation with clear functionality boundaries.

• PROBLEM: There are no clear boundary between Socket and ActiveConnections.
  Socket know too many details of a connection and ActiveConnections is basically a "proxy" object. Since a lot of functionality is in a Socket, it's impossible to write continuations, because of the Rust borrowing rules. E.g. to send heartbeat packets, you have to collect them first (heap allocation) and then send them, instead of iterating over packets and sending immediately (no heap allocation).
• SOLUTION: New types: SocketController and ConnectionController is introduced.
  • SocketController owns all core components: active connections, connection handler, event sender/receiver, socket sender/receiver. It is connection agnostic, it only stores a list of active connections, but doesn't do any operations on them. It only has one method manual_poll that does something useful: gets packets and events, and pass all connection related actions to a ConnectionController along with a connection.
  • ConnectionController Controls all aspects of the connection and owns only required components to process it:
    • Processes incoming data (from a socket) or events (from a user).
    • Updates connection state: resends dropped packets, sends heartbeat packet, etc.
    • Creates new connections.
    • Checks if the connection should be dropped.

2. Separated unit tests vs integration tests.

• PROBLEM: There are multiple issues with tests located in src/net/socket.rs:
  • Sometimes some tests just randomly fail. They are like unit tests, while at the same time they are integration tests because they use real implementation of UdpSocket, that depends on a state of an OS.
  • Some tests are testing things, that should instead be tested elsewhere. Few examples:
    • fragmentation_send_returns_right_size this is connection detail, and Socket shouldn't care about packet size or what is "fragmented" packet, etc...
    • ordered_16_bit_overflow also a connection detail. More importantly, since it is computation heavy it takes ~11sec on my machine to run it, because of the overhead by testing through Socket interface instead of testing directly.
• SOLUTION: By convention, unit tests shouldn't depend on any external components, they either fail or succeed 100% of the time. Rust, by convention, separates unit tests and integration tests in different directories.
  In order to make a majority of tests a unit tests, few traits were introduced: SocketSender and SocketReceiver and by using these traits SocketController and ConnectionController became socket type agnostic. So actual changes involved:
  • majority of tests was converted to unit tests by using fake socket implementation.
  • some tests, that was actually integration tests was improved and moved to test/basic_socket_test.rs.
  • some tests were moved to VirtualConnection because they were more related to an actual connection.

BREAKING CHANGES:

• Removed set_link_conditioner from Socket. It doesn't make sense to provide this test function in public API. Why would any sane user need to drop some random packets?

Although this looks like a big PR, it actually reduced the number of relevant code. By moving different parts to different places, it allowed writing cleaner code.

This is the last PR to prepare for #246 integration.

EDIT
A lot of changes were made during the process

[kstrafe]

See comments. I'm not sure about the Controller aspect. Why not just call it a normal, distinguishable thing? The Controller part is just noise.

[fraillt]

I'm not entirely happy with SocketController and ConnectionController names either :) During development I used SocketHandler and ConnectionHandler, but decided that controller was abetter fit.
It is the hardest part, to properly name these types and I need your help on this.

[TimonPost]

I love the concepts introduced in this PR

• A fake socket implementation for our tests
• Separation of the unit tests, because you'r right that they were mixed

A couple of questions

• Is our Socket renamed to SocketController?
• I am not sure what to think about the Controller/Handler names, in general, those names are bad practice. Because they are to general. But as you said, you need help with this. Mmmm, I do need to think about that a bit.

[fraillt]

Socket is not renamed, it basically wraps SocketController.

[fraillt]

Build is failing in jenkins, probably due to older clippy version.
Maybe could add addition jenkins stage, "Print Environment" before "Check Formatting"?
that would have few commands like:

cargo --version
cargo clippy --version
cargo fmt --version

What do you think?

[TimonPost]

Yea we should have something like that

[codecov]

Codecov Report

Merging #258 into master will increase coverage by 0.18%.
The diff coverage is 97.27%.

@@            Coverage Diff             @@
##           master     #258      +/-   ##
==========================================
+ Coverage    97.6%   97.79%   +0.18%     
==========================================
  Files          26       28       +2     
  Lines        2796     2673     -123     
==========================================
- Hits         2729     2614     -115     
+ Misses         67       59       -8

Impacted Files	Coverage Δ
src/net/quality.rs	97.91% <ø> (ø)	⬆️
src/lib.rs	100% <ø> (ø)	⬆️
src/packet/process_result.rs	100% <ø> (ø)	⬆️
src/packet/outgoing.rs	98.59% <ø> (ø)	⬆️
src/net/link_conditioner.rs	100% <ø> (ø)	⬆️
src/test_utils/fake_socket.rs	100% <100%> (ø)
src/test_utils/network_emulator.rs	95.45% <95.45%> (ø)
src/net/connection_manager.rs	97.1% <97.1%> (ø)
src/net/virtual_connection.rs	97.4% <97.22%> (-0.04%)	⬇️
src/net/connection_impl.rs	98.52% <98.52%> (ø)
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0121dc...024690b. Read the comment docs.

[fraillt]

I tried to add new stage "Environment Info" in Jenkinsfile, but it didn't appeared in CI.
So I restored it.

BTW. Socket -> set_link_conditioner on is now behind a feature flag tester.

[jstnlef]

Looks as though this is making Socket a facade and the real implementation lives in SocketController. Given this, maybe SocketImpl or something of the like is more appropriate.

[TimonPost]

Comments in functions should we do them with lower case or capital. There is a lot of inconsistency with that. We should choose one of those. I don't have any blocking comments. And I like the introduced features like the emulated socket.

[fraillt]

Indeed, lower case vs upper case in function bodies are very similar.
I ran this in a root directory on master branch:
grep -r -i --include \*.rs " // " | cut -d $' ' -f 2- | cut -d '/' -f 2- | cut -c 3-3 | sort | uniq.exe -c | sort

• 104 starts with upper case.
• 87 starts with lower case.
  Most of the comments are inside function bodies.

[TimonPost]

Don't mind them for now. That can be done in another PR.

[fraillt]

This commit addresses a lot of issues.
A lot of changes were made, but there is the gist of them:

• SocketImpl is now ConnectionManager<TSocket: DatagramSocket, TConnection: Connection>. It is a fully generic type, that depends on a socket and connection.
• new trait Connection, it allows to implement a concept of connection, that is used by ConnectionManager, it also defines types of send and receive events.
• instead of separate SocketSender and SocketReceiver traits, one DatagramSocket was introduced, this allowed to simplify some things.

I feel that the current state is much better now, but there are a few things that I'm not entirely happy with:

• to preserve backward compatibility addition trait was introduced ConnectionEventAddress. This was a necessary step because ConnectionManager doesn't know anything about types of connection events. Code could be simplified further if all events will be wrapped in this type:

  pub struct ConnectionEvent<Event: Debug>(pub SocketAddr, pub Event)

• ALL methods of Connection accepts a special trait, that does 3 things:
  • returns a config
  • sends an event
  • sends a packet
    I named this trait a ConnectionMessenger... I could probably remove this entirely, but then all methods of a connection would accept additional 2 parameters for each method.

[jstnlef]

I'm alright with this if @TimonPost is alright with it. I am a little leery about too much abstraction but all in all this isn't too bad. Good work.

[TimonPost]

I love the introduced abstractions. You've done great work. Lgtm!