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

Add support for "optimize tcp buffers" option per profile. #3137

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

notsure2
Copy link

  • It tunes down the local sslocal listener socket buffers which is the right thing to do in 99.999% of cases of mobile app.

  • It additionally also tunes down the remote socket buffers of sslocal when a plugin is used. This is needed to ensure there's no bufferbloat between sslocal and the plugin itself.

  • Ideally the plugin on its side should also choke its local listening socket buffers and leave only its remote side socket buffers unrestricted.

  • An additional follow up fix is needed which is to use a version of shadowsocks-rust which implements the buffer controls on the listening socket side. I have opened a PR there: Set the incoming socket buffer sizes properly on the listen socket. shadowsocks-rust#1381

  • Given that ss-android is using an older release of ss-rust. The PR will probably need to be backported to 1.5.x branch, or ss-android needs to upgrade to latest ss-rust after my PR is merged and a release is made.

  • This fixes the following issue which happens when using shadowsocks with or without a plugin:

  • Large file uploads immediately jump to 80-90% and hang because the unrestricted local socket of ss, copies most or the file inside it giving a premature acknowledgement to the app, then the app hangs while the actual proxy uploads the file bit by bit (this issue is highly apparent on slow upload internet condition). Most apps will consider this hang as a timeout and fail to upload.

  • This issue affects for example Whatsapp Google Drive backup which fails to upload while shadowsocks is working when the backup size becomes big enough. It also causes failures to google drive sync.

  • I added translation for all languages by carefully finessing google bard and translating back and forth multiple times until both sides give the same result, feel free to improve them.

 - It tunes down the local sslocal listener socket buffers which is the right thing to do in 99.999% of cases of mobile app.
 - It additionally also tunes down the remote socket buffers of sslocal when a plugin is used. This is needed to ensure there's no bufferbloat between sslocal and the plugin itself.
 - Ideally the plugin on its side should also choke its local listening socket buffers and leave only its remote side socket buffers unrestricted.
@Mygod
Copy link
Contributor

Mygod commented Aug 28, 2024

Hi please see here for the translation contribution: https://discourse.shadowsocks.org/t/poeditor-translation-main-thread/30

Is there a reason to still use the default buffer size? Why not enforce this size?

@notsure2
Copy link
Author

@Mygod Thank you for looking at this PR.

The small buffer size should be only incoming socket if plugin is not used, and in case of plugin used both incoming and outgoing socket to plugin should have their buffers stripped down.

I'm not sure what to do about translation ? I should remove from PR and add in that POEditor website ?

@Mygod
Copy link
Contributor

Mygod commented Aug 28, 2024

Only include the English texts and add them later.

Thanks for the PR but I think if the issues are real then shadowsocks-rust should probably provide a better default instead. @zonyitoo Thoughts?

@notsure2
Copy link
Author

I kind of disagree it will be a breaking change of behavior to all users who are not aware the effect of this change, that's why I don't even default this setting to on. The goal of this change is fix upload buffer bloat by removing buffer between app and ss and between ss and cloak in upload direction.

@notsure2
Copy link
Author

notsure2 commented Sep 1, 2024

@Mygod done

@Mygod
Copy link
Contributor

Mygod commented Sep 1, 2024

So is this a Cloak specific issue? Other plugins don't seem to have this problem.

@notsure2
Copy link
Author

notsure2 commented Sep 1, 2024

So is this a Cloak specific issue? Other plugins don't seem to have this problem.

Negative, this affects all plugins actually. It's most visible when using a connection with a slow upload. It looks like this, try to upload a large file with ss or ss + plugin without optimize buffers, on a slow upload internet.

What you will see, upload will jump to a high % immediately, then slowly actually upload to the background: Cause is that the browser copied to ss buffer very quickly and ss accepted it because default buffering is huge, but actually the underlying upload can't handle it, so the upload just hangs a for a long time with no progress until it catches up, then continues at the real speed to the end.

Many upload controls will cancel the upload with "Timeout" error if no bytes move for a long time (which happens in this case due to the buffer bloat at the local ss).

App ----------------> ss ------------------> internet
        (balloon here)         

In ss alone the balloon must be small or it will inflate with data much faster than ss can empty it to the internet giving a false impression of upload speed to the app potentially leading to timeouts as the app thinks the connection stalled.

App ----------------> ss ------------------> plugin --------------------> internet
        (balloon here)         (balloon here)

Same problem here, the idea is all buffers from the app to the internet except the last one must be set very small so that the app gets a correct TCP backpressure and correct "feeling" of upload speed and works properly.

PS: This problem doesn't affect download, only upload

@Mygod
Copy link
Contributor

Mygod commented Sep 2, 2024

It seems that the default buffer size is 16383*20 bytes unless I'm mistaken? This probably wouldn't cause the issue you described unless I'm mistaken.

@notsure2
Copy link
Author

notsure2 commented Sep 2, 2024

That's 20 times what I set :-) I set to just 16384. Also default is whatever is core_wmem i think in kernel, for localhost it's very big.

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

Successfully merging this pull request may close these issues.

2 participants