Skip to content
This repository has been archived by the owner on Nov 15, 2019. It is now read-only.

Improve uTP #445

Closed
wants to merge 2 commits into from
Closed

Conversation

vinipsmaker
Copy link
Contributor

This updates #439 by adding more protection on the crust side of things. To close the issue for good, updates are also required on the rust-utp side.

Review on Reviewable

This change will decrease the chance we're not reading from socket when
a packet arrives (i.e. decrease the chance to a packet loss event
occurs).

It shouldn't be needed, but rust-utp isn't being very reliable. At some
point, it might be required to patch rust-utp to update the congestion
control algorithm and the strategy to when resend a packet.
This is a mechanism to protect from unreliability of rust-utp.
Currently, timeout is 5min, but can be changed to better suit Routing
needs. Thanks to only high-level events being detected (and
`cbor::Decoder` taking ownership of our chance to intercept "less than a
whole message received" events like "a bunch of bytes received"), this
mechanism may not be optimal if the crust user is hoping to send bigger
messages.
@maidsafe-highfive
Copy link

r? @inetic

(maidsafe_highfive has picked a reviewer for you, use r? to override)

@vinipsmaker
Copy link
Contributor Author

r? @maqi

@maidsafe-highfive maidsafe-highfive assigned maqi and unassigned inetic Dec 10, 2015
@maqi
Copy link
Contributor

maqi commented Dec 11, 2015

could you link the required rust-utp updates as well ?
Also, a detailed test report of the failure rate before and after this update will be preferred to be included as well.
Otherwise it will be hard to tell whether this update is necessary.

And, the PR #442 is preferred to be merged before this PR,
as the testing code of network using utp is in that one.

@vinipsmaker
Copy link
Contributor Author

could you link the required rust-utp updates as well ?

I started working on this after I submitted this PR. The idea is to not leave crust in a so bad situation while we fix the real issue within rust-utp.

also, a detailed test report of the failure rate before and after this update will be preferred to be included as well.

Well, the current issues won't happen because if rust-utp misbehave, crust will issue a "connection loss" event. We "just" exchange one error for another. Connection loss is expected in the wild and layers above crust will have to handle them anyway. With the patch, they'll have the chance to not hang thanks to misbehaving rust-utp.

@maqi
Copy link
Contributor

maqi commented Dec 11, 2015

could you link the required rust-utp updates as well ?

I started working on this after I submitted this PR. The idea is to not leave crust in a so bad situation while we fix the real issue within rust-utp.

If so, I will prefer merge your rust-utp work first then merge this PR, or merge them at the same time.

also, a detailed test report of the failure rate before and after this update will be preferred to be included as well.

Well, the current issues won't happen because if rust-utp misbehave, crust will issue a "connection loss" event. We "just" exchange one error for another. Connection loss is expected in the wild and layers above crust will have to handle them anyway. With the patch, they'll have the chance to not hang thanks to misbehaving rust-utp.

However, the current crust network test only covers TCP, and thispromised behaviour is not tested. It is good crust raise a connection loss instead of just hanging, but we need a report here of such failing rate is acceptable.

In summary, could you please change the title of this PR to WIP, and once your rust-utp work is done, put a summary report here so this PR can be properly verified?

@maqi maqi mentioned this pull request Dec 11, 2015
@vinipsmaker vinipsmaker deleted the improve-utp branch March 24, 2016 08:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants