Syncing documentation #260
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
Since the protocol specifies the use of 16 bit counters, it's best to reflect these in the API as well.
Turn arranging usize into u16
The expected index is a u16 at the protocol level, but the arranging implementation for orderingstream does not take this into account. This caused sending over 2**16 ordered packets to simply default to the `None` case and get ignored. This patch solves that issue.
Previously we would only store packets with indices higher than the expected index, this is obviously not going to give us good results when we receive packets 0..100 while we are currently expecting index 65500. What this patch does is that it stores all incoming packets with the condition: expected < index < expected + u16::max/2 With wrapping add this becomes slightly more complex but the gist is the same.
Fix ordering arranging handler for >65536 packets
The previous patches would erroneously skip the value 32767 and drop the next packet (32768). This patch and test ensures that this doesn't happen.
Sending over 65536 packets would previously just saturate the top_index which caused it to not accept any more packets. This patch shortens the sequence acceptance by half but always loops around the max u16 value. This means that at top_index = 0, all sequenced packets 0-32768 are accepted, while at top_index = 50000, all sequenced packets 50000-65535 and 0-17233 will be accepted.
Ensure that `self.remote_ack_sequence_num` is always increasing
crossjob for building and testing laminar
The connection is disconnected if we have N packets-in-flight simultaneously. Under normal usage we expect packets to be acked regularly so that our packets-in-flight size is relatively small.
* Most changes are in `VirtualConnection` struct to make `process_incoming` and `process_outgoing` results make iterable. * Introduced a new `PacketInfo` type. * Bugfix covered by a test, when `last_sent`, was not set when sending `Unreliable` packets. * Removed a lot of warnings when running `cargo clippy --tests`.
Codecov Report
@@ Coverage Diff @@
## master #260 +/- ##
=======================================
Coverage 97.75% 97.75%
=======================================
Files 28 28
Lines 2673 2673
=======================================
Hits 2613 2613
Misses 60 60
Continue to review full report at Codecov.
|
jstnlef
reviewed
Oct 11, 2019
| 2. Hard Coding | ||
| - Don't hard code values anywhere | ||
| - Use the ‘NetworkConfig’ type for common network settings, use consts or parameter input | ||
| - Use of lazy_static is acceptable but first make sure you can’t fix the issue in other ways | ||
| 3. Code markup | ||
| - Keep files small. Better have small files with small pieces of logic than having one file with 1000 lines of logic with multiple types/structs etc. Note that I speak of logic, tests not included | ||
| - No panics/unwraps in the main codebase, but they are accepted in tests | ||
|
|
||
| ## Import Reordering | ||
| All imports are semantically grouped and ordered. The order is: |
There was a problem hiding this comment.
Do we have a way to enforce this? It seems awfully pedantic if we don't have a programmatic way of both performing the transform and validating it.
There was a problem hiding this comment.
I don't believe so. However, as noted the IntelIJ rust plugin is able to do this. So once in a while, we can run that to validate everything is ordered correctly. I have no idea how to enforce it in some way.
| /// successfully received. | ||
| pub fn ack_bitfield(&self) -> u32 { | ||
| let most_recent_remote_seq_num: u16 = self.remote_sequence_num(); | ||
| let mut ack_bitfield: u32 = 0; | ||
| let mut mask: u32 = 1; | ||
|
|
||
| // Iterate the past REDUNDANT_PACKET_ACKS_SIZE received packets and set the corresponding | ||
| // iterate the past `REDUNDANT_PACKET_ACKS_SIZE` received packets and set the corresponding |
There was a problem hiding this comment.
Curious why would wouldn't capitalize these as well?
There was a problem hiding this comment.
Because there are also very small '//' comments, and those are often not a complete sentence. I do think that either way we should do everything capitalized or with a lowercase. This PR has everything lower case. It is a matter of preference.
jstnlef
approved these changes
Oct 12, 2019
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.
//