-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 client: Raise idle timeout to 5 minutes #3624
Conversation
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.
我觉得这项改动是合理的,因为 *ray 的默认超时就是五分钟, 但应只留 MaxIdleTimeout,把另外两个删掉,默认即可,握手五秒已经够用了 |
I looked at wireshark yesterday to figure out if it is very obvious, but the h2/h3 connection reuse makes it a bit too difficult, and CDN's own idle timeout interferes as well. I think what is needed is a randomized keepalive, but maybe in another layer (in vless or mux.cool?) so that it is solved for all transports. There were also some users who complained that their SSH session does not survive for more than a few minutes, so they should be happy about this as well. Anyway, for another time. |
Actually, quic-go's default quic config and the default quic config in http3 are different. Here is the actual default config for http3 package So in practice, this PR changes the following settings, because it sets an explicit QUICConfig:
anyway, it still seems reasonable to me. |
对这个有点感兴趣,这是指服务端可以发起双向流吗, |
I think it's mostly because quic config is used for both server and client applications. It makes a bit more sense on the server. I don't think server-initiated streams are useful, even if CDN supports full quic forwarding. It's only interesting that there are bidirectional streams, not that a stream is initiated by the server. But not 100% sure. |
因为确实错了,比如说 CDN 不支持 streaming request 是怕客户端攻击,但若 bidirectional stream 是服务端发起的就不会是攻击了 |
所以说其实现在还是有 KeepAlive 的,只是从 3 秒改成了 10 秒, 我觉得除了 MaxIdleTimeout,其它两个先和 quic-go H3 默认值保持一致吧, |
发现 MaxIncomingStreams 为 0 时会被 quic-go H3 自动设成 -1: |
不过我觉得还是三个都写上吧更直观,记得注释,还有 H2 和 H1 的 timeout 你看看要不要一起改,还有服务端有没有设 timeout |
I was under the impression that I will take another look in a few days and see if I understand all the defaults correctly.
Where did you find this? I did not find the number 3 at all. revisiting h1 and h2 also makes sense |
这是http3的规定 不允许服务器发起双向流 (RFC 9114 6.1) |
|
现阶段纠结这些好像意义不大毕竟以后都是要换成uQUIC的 手动学舌效果一般 顺便现在的uquic除了firefox esr其他的好像并不完美 然后这个quic指纹实际流量占比又少得可怜 |
我们摆明了用的就是 quic-go H3, uQuic 现在基于旧版 quic-go 还有 performance 问题:#3550 (comment) ,所以等它 v0.1.0 |
所以我刚用 Wireshark 看了下 SplitHTTP 的 quic-go 大多数包都有 DCID, |
Isn't it better to use connIdle from Policy settings as idle timeout of splithttp? |
It should not be handled in the transport layer (actually, the design of the entire policy is not good) |
@RPRX check if this logic makes sense to you. tested everything with wireshark. |
对吧,因为 keepAlive 包的长度也是很固定的,我还有个小疑问是这些 keepAlive 包是基于主连接还是子连接? |
@@ -131,7 +151,8 @@ func getHTTPClient(ctx context.Context, dest net.Destination, streamSettings *in | |||
DialTLSContext: func(ctxInner context.Context, network string, addr string, cfg *gotls.Config) (net.Conn, error) { | |||
return dialContext(ctxInner) | |||
}, | |||
IdleConnTimeout: 90 * time.Second, | |||
IdleConnTimeout: connIdleTimeout, | |||
ReadIdleTimeout: h2KeepalivePeriod, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里?
(deleted some comment, ignore)
I tested again, yes the size is also constant. However, what I meant is that the keepalive interval (h2 ping, not TCP, to be clear) is different between browsers. So it is strange to set The keepalive packets happen every N seconds as observed on wireshark, so for the outer connection, opening more streams inside doesn't change it. |
If there are no further comments I will merge this PR tomorrow. |
这也是我想说的,并且如果不用 XTLS 的话其实多加密一次也很明显,而且小包不止这个,
|
I don't understand the question
This is from http2 package. I think it's the right parameter. But the name "idle timeout" is strange to me, and in quic the equivalent is called KeepAlivePeriod, so I renamed the const to this. |
@yuhan6665 @Fangliding 以后 commit title 并列写多个 changes 就用 85e2ebc 的格式:英文逗号分隔,每一小段首字母大写
|
Copy the settings from QUIC transport. See
#3565 (comment) -- Tested with CDN and it seems better than before (fewer reconnections)
Keep-alive packets are still turned off, so the original concern of that PR should still be resolved.