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

Fix: do not leave empty connetion #3565

Merged
merged 2 commits into from
Jul 20, 2024
Merged

Fix: do not leave empty connetion #3565

merged 2 commits into from
Jul 20, 2024

Conversation

chise0713
Copy link
Contributor

https://t.me/projectXray/3782602

Co-authored-by: Fangliding <[email protected]>
Co-authored-by: xqzr <[email protected]>
Co-authored-by: ll11l1lIllIl1lll <[email protected]>
@Fangliding Fangliding merged commit 57248d3 into XTLS:main Jul 20, 2024
36 checks passed
@Fangliding
Copy link
Member

Brief:
xq发现h3有太多的空包 昨天没引起重视 今天又抬上来说了 大火意识到quic可能打开了多个连接 但是并未关闭 一直发keepalive造成抓包结果
最后发现是因为代码里设置了一个连接保活90s并且3秒一个心跳包这么激进的一个保活参数 当时以为这个连接是复用的所以调这样 实际上h3没有流复用 导致每一个请求都开一个quic连接然后都被疯狂发包保活
0RTT删掉是因为实际上这个特性激活不了 需要用它内置的其他方法发起GET或者HEAD请求才能用到

@chise0713 chise0713 deleted the patch-1 branch July 20, 2024 17:56
@Mfire2001
Copy link

0RTT was deleted because this feature cannot be activated. It needs to use other built-in methods to initiate GET or HEAD requests to use it.

@Fangliding
Are u sure it cannot be used?in quic-go docs it says in order for 0rtt to work client must support session resumption and I havnt seen anyone activate that
in tlsconfig,and if using a cdn like cloudflare,i think 0rtt and session ressumption is off by defualt

@Fangliding
Copy link
Member

Fangliding commented Jul 20, 2024

0RTT was deleted because this feature cannot be activated. It needs to use other built-in methods to initiate GET or HEAD requests to use it.

@Fangliding Are u sure it cannot be used?in quic-go docs it says in order for 0rtt to work client must support session resumption and I havnt seen anyone activate that in tlsconfig,and if using a cdn like cloudflare,i think 0rtt and session ressumption is off by defualt

It requires both session resumption and using http3.MethodGet0RTT/http3.MethodHead0RTT
Maybe we can use Get0RTT but we still need POST to send request, so this is meaningless

@mmmray
Copy link
Collaborator

mmmray commented Jul 20, 2024

we can use GET for both upload and download, and distinguish upload and download on the server by path. it can be done in a backwards-compatible way. what I am not sure about however is if "GET with a request body" is a legitimate construct that will pass CDN.

@Fangliding
Copy link
Member

Fangliding commented Jul 20, 2024

we can use GET for both upload and download, and distinguish upload and download on the server by path. it can be done in a backwards-compatible way. what I am not sure about however is if "GET with a request body" is a legitimate construct that will pass CDN.

Like WS early data, maybe we can put some data in request header?

@mmmray
Copy link
Collaborator

mmmray commented Jul 20, 2024

I regret my words I mean yes, that probably works.

@APT-ZERO
Copy link

'GET with a request body' maybe not supported by some CDN or software
maybe even considered as a malicious request and blocked
Isn't Header or Get Query enough?

@mmmray
Copy link
Collaborator

mmmray commented Jul 20, 2024

yeah i think it's the same suggestion as fangliding

@RPRX
Copy link
Member

RPRX commented Jul 21, 2024

所以下次能不能 Squash,否则我都找不到对应的 PR 去 review,我给 @yuhan6665 说过很多次了

@RPRX
Copy link
Member

RPRX commented Jul 21, 2024

quic-go 没复用连接这个问题是比较诡异的,毕竟多个 POST 时它应该是复用了连接,但不知道为什么代理个新连接它就不复用了

@mmmray
Copy link
Collaborator

mmmray commented Jul 21, 2024

所以下次能不能 Squash,否则我都找不到对应的 PR 去 review,我给 @yuhan6665 说过很多次了

there is one commit for this PR on main branch. it looks squashed to me. do you mean to keep the PR number in the commit message or something like that? can still go to it from here

@RPRX
Copy link
Member

RPRX commented Jul 21, 2024

do you mean to keep the PR number in the commit message or something like that? can still go to it from here

是的,用这么久 GitHub 我今天才知道

@RPRX
Copy link
Member

RPRX commented Jul 21, 2024

总之这个 commit 的 title 也是看起来一头雾水的,至少要指明是 SplitHTTP 吧,麻烦 @mmmray 把它改成这样:

SplitHTTP: Remove unnecessary keepalive (#3565)

mmmray pushed a commit that referenced this pull request Jul 21, 2024
Remove keep alive since quic-go/http3 doesn't support stream reuse
Discussion see https://t.me/projectXray/3782492

Co-authored-by: Fangliding <[email protected]>
Co-authored-by: xqzr <[email protected]>
Co-authored-by: ll11l1lIllIl1lll <[email protected]>
@mmmray
Copy link
Collaborator

mmmray commented Jul 21, 2024

ok done this force-pushing business is really disgusting, it causes noise in open PRs

@Fangliding
Copy link
Member

Fangliding commented Jul 21, 2024

所以下次能不能 Squash,否则我都找不到对应的 PR 去 review,我给 @yuhan6665 说过很多次了

一直都是Squash and merge啊 rebase那个选项会显示n个commit(如果有)然后xxx authored and xxx commited 除非是很简单的修改不然一般都不会用

虽然读后面的聊天记录知道了是没有留pr number em 我也觉得这标题不对 所以重写commit message的时候标题是直接ctrl+a的 下次会注意一点 一方面它确实可以直接从那mmm说的那地方点进来

还有如果只想squash的话可以从仓库设置里关掉(create merge commits就已经被关掉了)

@RPRX
Copy link
Member

RPRX commented Jul 21, 2024

Brief: xq发现h3有太多的空包 昨天没引起重视 今天又抬上来说了 大火意识到quic可能打开了多个连接 但是并未关闭 一直发keepalive造成抓包结果 最后发现是因为代码里设置了一个连接保活90s并且3秒一个心跳包这么激进的一个保活参数 当时以为这个连接是复用的所以调这样 实际上h3没有流复用 导致每一个请求都开一个quic连接然后都被疯狂发包保活 0RTT删掉是因为实际上这个特性激活不了 需要用它内置的其他方法发起GET或者HEAD请求才能用到

结果 h3 是有复用 quic 的 #3560 (comment)

不过我觉得这个 KeepAlivePeriod 仍然没必要,可以删掉,Allow0RTT 是“Only valid for the server.”,也应该删掉

MaxIdleTimeout 不是连接保活 90s,而是不活跃超时,“If this value is zero, the timeout is set to 30 seconds.”

HandshakeIdleTimeout 是“If this value is zero, the timeout is set to 5 seconds.”,我觉得这些都可以先用默认值,不然特征有点多

@mmmray
Copy link
Collaborator

mmmray commented Aug 2, 2024

I took a look at connection lifetimes again today. I don't care about keepalive (it can be disabled), but I think the new MaxIdleTimeout (30s) is a bit too short. Other transports allow an idle tunneled TCP connection to stay open for at least a few minutes, but splith3 tears it down exactly after 30s.

It seems to me the defaults from the QUIC transport would be very reasonable:

KeepAlivePeriod: 0,
HandshakeIdleTimeout: time.Second * 8,
MaxIdleTimeout: time.Second * 300,

what do you think?

mmmray added a commit to mmmray/Xray-core that referenced this pull request Aug 2, 2024
Copy the settings from QUIC transport. See
XTLS#3565 (comment) --
Tested with CDN and it seems better than before (fewer reconnections)

keep-alive packets are still turned off, so the original concern should
be resolved.
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.

6 participants