Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow limiting download speed #15

Closed

Conversation

tearfur
Copy link
Member

@tearfur tearfur commented Dec 20, 2023

Part 1 of Fixing transmission/transmission#6361.

Part 1 and part 2 combined will allow Transmission to mimick the behaviour of the TCP network stack when the link is saturated:

  1. Transmission buffers packets when the bandwidth "cannot" accomodate them.
  2. Drop our advertised receive window size as Transmission's read buffer fills up.
  3. libutp will "drop" incoming packets when Transmission's buffer is "full".

This PR is responsible for point 3: Dropping incoming packets when our read buffer is full.

I am aware that we are trying to make as little changes as possible to our libutp fork. Fortunately, in my tests, the uTP download bandwidth limit still works pretty well even if we merge part 2 only. But in theory, this will only hold if our peers respect our advertised uTP receive window: Without this PR, if we have naughty peer(s) that send more bytes at us than our advertised uTP receive window allows, we will have no way to stop them from filling up our peer-io read buffer.

@tearfur
Copy link
Member Author

tearfur commented Dec 20, 2023

ping @ckerr @mikedld to make sure this PR will be noticed.

@tearfur
Copy link
Member Author

tearfur commented Dec 20, 2023

Part 2 (transmission/transmission#6416) of this series will complete the fix. If this PR gets merged, I will update part 2 to bump libutp as well.

@tearfur tearfur marked this pull request as draft December 20, 2023 08:24
@tearfur
Copy link
Member Author

tearfur commented Dec 20, 2023

Jumped the gun too soon, please hold off from reviewing 😅

Edit: Ready now.

@ckerr
Copy link
Member

ckerr commented Dec 23, 2023

@tearfur is a libutp patch for this unavoidable?

I'd really prefer to avoid changing libutp behavior if possible. We rarely touch this repo and this would be the first commit that actually changes any of its behavior.

If it's absolutely unavoidable then ... maybe?

@tearfur
Copy link
Member Author

tearfur commented Dec 23, 2023

I am aware that we are trying to make as little changes as possible to our libutp fork. Fortunately, in my tests, the uTP download bandwidth limit still works pretty well even if we merge part 2 only. But in theory, this will only hold if our peers respect our advertised uTP receive window: Without this PR, if we have naughty peer(s) that send more bytes at us than our advertised uTP receive window allows, we will have no way to stop them from filling up our peer-io read buffer.

I think we have the option to not merge this. I've already implemented a failsafe in Part 2 that keeps the read buffer from ballooning indefinitely, so the biggest impact I can think of is just that Transmission will download faster than the download limit if we get a peer that does not respect our receive window size.

@ckerr
Copy link
Member

ckerr commented Dec 24, 2023

@tearfur, apologies for not reading your writeup all the way through. 😞

Though, I do also appreciate the extra description today that spells out a little more clearly what happens if a peer disregards the advertised speed.

For now, let's try to keep the fix in libtransmission and touch libutp only if there are problems reported in the wild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants