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

SplitHTTP: Do not produce too large upload #3691

Merged
merged 3 commits into from
Aug 17, 2024
Merged

Conversation

mmmray
Copy link
Collaborator

@mmmray mmmray commented Aug 16, 2024

Since #3603 and #3611, iperf and speedtest are triggering "too large upload" by the server. This is because v2ray's MultiBuffer pipe can actually return data larger than the configured size limit.

I'm surprised nobody noticed it so far. In principle, any heavy upload could disrupt the entire connection. In its infinite wisdom, speedtest.net hides such errors and only shows low upload instead. I only noticed this issue myself when inspecting server logs.

Since XTLS#3603 and
XTLS#3611, iperf and speedtest are
triggering "too large upload" by the server. This is because v2ray's
MultiBuffer pipe can actually return data larger than the configured
size limit.

I'm surprised nobody noticed it so far. In principle, any heavy upload
could disrupt the entire connection. In its infinite wisdom,
speedtest.net hides such errors and only shows low upload instead. I
only noticed this issue myself when inspecting server logs.
@APT-ZERO
Copy link

That 'ed' in the Path is interesting
Is it needed to enable Early Data for SplitHTTP? isn't it 0-rtt by default?

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 17, 2024

@APT-ZERO expand the code further, there's a code comment. the test is just meant to demonstrate that querystrings don't break the transport entirely. it serves no purpose, but the test was incomplete. (it's not related to this PR just some random unrelated change)

@Fangliding
Copy link
Member

Fangliding commented Aug 17, 2024

That's why some users report that increasing the maximum upload bytes on server can improve upload speed significantly
I used to suspect that this was some HTTP issue so client send bigger packet, but I didn't expect the reason to be like this

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 17, 2024

by the way, i'm struggling with MultiBuffer pipe. it has a strange API due to performance concerns, and a lot of code seems to rely on the behavior of WithSizeLimits(0). It would be more appropriate to use a different buffered pipe but it would mean more dependencies. Maybe I will write another one someday.

@mmmray
Copy link
Collaborator Author

mmmray commented Aug 17, 2024

also I suppose this is why @MHSanaei added different defaults to splithttp: MHSanaei/3x-ui@d319476#diff-b5ef5f7bdd4bc564ca919c23529956b3402c3065543fbe2f773088b6893cb467R559

(most of those options have no real effect on the server, but the bump of MaxEachPostBytes works around this bug)

@mmmray mmmray merged commit 160316d into XTLS:main Aug 17, 2024
36 checks passed
@mmmray mmmray deleted the sh-pipe-limits branch August 17, 2024 11:02
@alireza-2030
Copy link

also I suppose this is why @MHSanaei added different defaults to splithttp: MHSanaei/3x-ui@d319476#diff-b5ef5f7bdd4bc564ca919c23529956b3402c3065543fbe2f773088b6893cb467R559

(most of those options have no real effect on the server, but the bump of MaxEachPostBytes works around this bug)

Because the 3x-ui panel has two types of export.

1- Normal export, for example, to vmess or vless.

2- Export as json sublink!

These parameters that are set in the server (panel) are placed in json and given to the user (as a sublink).

@RPRX
Copy link
Member

RPRX commented Aug 23, 2024

其实我早就测出了测速上行偶尔有速度偶尔没速度,本来还以为是网络问题,所以在群里看到这个 #3643 (comment) 时挺关注的

zxspirit pushed a commit to zxspirit/Xray-core that referenced this pull request Aug 30, 2024
leninalive pushed a commit to amnezia-vpn/amnezia-xray-core that referenced this pull request Oct 29, 2024
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.

5 participants