Implement the ordering module #148
Comments
|
I will be doing a PR soon for implementing this. |
This was referenced Mar 1, 2019
|
Closing this because of #150 |
bors bot
added a commit
that referenced
this issue
Mar 30, 2019
150: Arranging module implementation r=fletcher a=TimonPost This PR implements the [arranging](https://github.com/amethyst/laminar/tree/master/src/infrastructure/arranging) module into laminar. A view steps where necessary to accomplish this. ## Changes In this section, I want to highlight the key points of what I changed and clarify the many changes. 1. Implemented Arranging of packets. 2. Removed Channels 3. Redesign Headers 4. Unit Tests Those issues are solved: #137 #148 ### Justifying so many changes. Don't worry, a lot of changes made are moved code, removed code, and unit tests. 1. Implemented Arranging of packets. This was my main priority and the points below are the ones who were infected by this and needed to be updated too. - [Removed Channels](#Channels) (- 314 LoC) - [Redisgning Headers](#PacketHeaders) (- 191 LoC, | + 247 LoC) - [Refactored Packetprocessing](#PacketProcessing) - Existing logic moved to own types (- 77 LoC | + 77 LoC) - Introduced some types to move some code out of `VirtualConnection` to (+ 329 LoC) I did this to make things easier, cleaner and to prevent duplication, prevent a 1500 line code file. - Changes in processing code (231 LoC); this was eventually the goal of my PR, and I think I kept it pretty close to our 200 boundaries ;). 2. [Unit Tests](LINK) (+ 755 LoC): For the unit tests, I made 755 addictions, which is about 41% percent of the changes, which is positive right? **Conclusion** It seems like there are a lot of changes but to put it into perspective: 41% unit tests, 14% packet header changes, 12% PR-oriented changes, 22% were oriented around moving code, we are left with 11% which are from comments and some namespace managing. ### Channels I removed the channels because they did not fit very well with the ordering and sequencing. The problem I encountered was that both the `ReliableChannel` and `UnrelialbeChannel` needed to access the `OrderingSystem` mutable and there were two options to do this: 1) Mutex<Arc<<ArrangingSystem>>; this is a no-go. 2) Passing a mutable reference to the functions of those channels This isn't ideal because I need to pass the mutable references to those functions even if those arguments are not used; e.g. Unreliable + Unordered, Reliable + Unordered, Also, the trait definition would become very strange because e.g. `ReliableChannel` should eventually have access to both SequencingSystem `OrderingSystem`, and to allow that we should need to some dynamic dispatch. 3) The channel idea was a bit weird, at first I thought it would be helpful, but I noticed that it was not very much needed at all. I noticed that if I would continue with the channels that I had to do some duplication of code and that processing would eventually become more difficult. If those channels are removed where did the logic in them go? I moved the code of those channels to VirtualConnection; it has two methods: [process_incomming()](https://github.com/TimonPost/laminar/blob/arranging_module_implementation/src/net/connection/virtual_connection.rs#L209), [process_outgoing()](https://github.com/TimonPost/laminar/blob/arranging_module_implementation/src/net/connection/virtual_connection.rs#L81). In there I take care of processing incoming packets. ### Packet Processing As we spoke of in [Channels](#Channel), the code is moved to VirtualConnection. We have two processes here that are of important: 1. Incoming Packet Handling  2. Outgoing Packet Handling  3. I removed `DeliveryType` and split it up into two enums: I did this because it would make processing easier. - [Ordering Guarantees](https://github.com/TimonPost/laminar/blob/arranging_module_implementation/src/packet/enums.rs#L32) ```rust pub enum OrderingGuarantee { /// No arranging will be done. None, /// Packet will be arranged in sequence. Sequenced(Option<u8>), /// Packet will be arranged in order. Ordered(Option<u8>), } ``` As you see `Sequenced` and `Ordered` are both containing an `Option`, this allows the user to pass in `None` as well. If `None` is specified then the default stream would be used; which is specified in `net::constants`. - [Delivery Guarantee](https://github.com/TimonPost/laminar/blob/arranging_module_implementation/src/packet/enums.rs#L5) ``` rust pub enum DeliveryGuarantee { /// Packet will be delivered Unreliable Unreliable, /// Packet will be delivered Reliably Reliable, } ``` **conclusion** I've decided to put all the processing logic into `VirtualConnection` I did a view attempts to separate some processing code because there was a lot of logic, however a lot of those attempts ended up unnecessarily boilerplate code or dirty code because the processing logic has lot's of common code and spreading this only increased the sharing of variables and such. ### Packet Headers For arranging, I had to modify the packet headers a bit. 1) I've introduced a new header called [ArrangingHeader](https://github.com/TimonPost/laminar/blob/arranging_module_implementation/src/packet/header/arranging_header.rs), this header contains two values: the `order_id` and the `stream_id`, those are needed to know at which 'stream' the ordering should be done specified on the given 'id'. 2) I removed the dependencies of headers to each other; `AcknowledgmentHeader` had a `StandardHeader`, `FragmentHeader` had an `AcknowledgmentHeader`. Until now this was valid, but, the new `ArrangingHeader` could be constructed with another header in two different ways: - `StandardHeader` -> `AcknowledgmentHeader` -> `ArrangingHeader`; in case of 'Reliable + Ordered' - `StandardHeader` -> `ArrangingHeader`; in case of 'Unreliable + Sequenced' I needed to make each header to be a totally independent header and to have a static size because the headers of a packet vary on the `DeliveryGuarantee` and `OrderingGuarantee`. Let's take a look at this diagram first. _Header Diagram_  As you see each packet type has its own header, we need to do some parsing - as we did before - to reconstruct the given headers when receiving a packet. The constructing and reconstructing of packets and headers is the main part why this PR is a little too big. I introduced two types to abstract away a bit of the processing code, and to prevent to much code in `VirtualConnection`: - [PacketReader](https://github.com/TimonPost/laminar/blob/arranging_module_implementation/src/packet/packet_reader.rs) Could be used to read the packet contents of laminar, where 'contents' is the headers and payload. - [OutgoingPacketBuilder](https://github.com/TimonPost/laminar/blob/arranging_module_implementation/src/packet/outgoing.rs#L12) A builder that could be used to construct an outgoing laminar packet, the packet includes the header and payload. - [Outgoing](https://github.com/TimonPost/laminar/blob/arranging_module_implementation/src/packet/outgoing.rs#L126) Enum for storing different kinds of outgoing types with data. ``` pub enum Outgoing<'a> { /// Represents a single packet. Packet(OutgoingPacket<'a>), /// Represents a packet that is fragmented and thus contains more than one `OutgoingPacket`. Fragments(Vec<OutgoingPacket<'a>>), } ``` - [OutgoingPacket](https://github.com/TimonPost/laminar/blob/arranging_module_implementation/src/packet/outgoing.rs#L107) ``` pub struct OutgoingPacket<'p> { header: Vec<u8>, payload: &'p [u8], } ``` As you might already see I introduced a packet which has lifetimes to some buffer. The idea behind this is, is that the data the user sends into laminar could be a reference until it is sent on the socket. The header could not be a slice, because those are dynamic and dependent on the packet type. ## Unit Tests The most changes about 41% were caused by new unit tests. To assure that my code is working, I have tests for packet order, different reliabilities, packet serialization, deserialization, etc. I could do some more but the PR-size held me back I will do some later. # Conclusion I know the size of this PR is big, I don't mind if some time has to be taken to review it. Although as I noticed before, not all changes are new code and not all file changes are real but most of them are caused by the rename `DeliveryType` to `DeliveryGuarantee`. Co-authored-by: Timon Post <timonpost@hotmail.nl>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In #145 the arranging module was added and not yet coupled with laminar. This should be implemented for laminar.
The text was updated successfully, but these errors were encountered: