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 client: Add xmux (multiplex controller) for H3 & H2 #3613

Merged
merged 13 commits into from
Sep 16, 2024

Conversation

ll11l1lIllIl1lll
Copy link
Contributor

@ll11l1lIllIl1lll ll11l1lIllIl1lll commented Jul 29, 2024

我不确定我有没有正确理解其工作原理,并且我写完之后才发现上游已经更新了一堆提交,可能我合并的时候也哪里改炸了。直觉告诉我这可能有资源泄露问题,至少好像能用,所以我提交了。
这里假定 http.Client 会对每个请求复用连接,我实际使用的时候也看起来像是对请求复用连接。所以 idk.

这个代码其实挺烂的,我稍后再改,或者还是等另一个人去以更好的方式实现吧,至少肯定不是我。

注:好像 Interval 更倾向于定时任务的间隔,如果是硬限制最小间隔可能 Delay 更符合表达习惯。并且可能那 3 个 sc 参数也应该移到 mux

@yuhan6665
Copy link
Member

感谢 PR
虽然目前只有 SH 能用 但是考虑一下我们要不要把选项并入 https://xtls.github.io/config/outbound.html#muxobject

@Fangliding
Copy link
Member

em 毕竟原理不一样 这个是复用client而不是mux.cool 所以放这还算河里吧

Copy link
Collaborator

@mmmray mmmray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just commenting on the implementation. maybe rprx wants more settings, but to me it seems this can resolve some quic instabilities in any case, so it's good in my opinion. thanks!

transport/internet/splithttp/config.proto Outdated Show resolved Hide resolved
transport/internet/splithttp/config.proto Show resolved Hide resolved
return muxMan.getClient(ctx, dest, streamSettings)
}

func createHTTPClient(ctx context.Context, dest net.Destination, streamSettings *internet.MemoryStreamConfig) *DefaultDialerClient {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was confused where this function was used, turns out it is directly pulled in from muxmanager

i suggest to make mux manager unit-testable (and add some tests), so it takes the function as an argument

transport/internet/splithttp/dialer.go Show resolved Hide resolved
transport/internet/splithttp/mux.go Outdated Show resolved Hide resolved
transport/internet/splithttp/config.proto Outdated Show resolved Hide resolved
@RPRX
Copy link
Member

RPRX commented Jul 30, 2024

Good job!这下又不用自己写代码了,晚点我 review 一下

@dantesieg
Copy link

Using xray version including these commits causes crash after a while when using on client, SplitHTTP with h3 alpn. If needed i can send my configs, i used new mux mechanism with h3. Server uses same version taken from Github Actions. I don't know if this helps. I just enabled new mux and tried "prefer_new", "prefer_reuse" and "prefer_existing", didn't change other options.

panic: connection already exists

goroutine 400 [running]:
github.com/quic-go/quic-go.(*connMultiplexer).AddConn(0x400007d0e0, {0x75a9ec4f40?, 0x40000702f8?})
	github.com/quic-go/[email protected]/multiplexer.go:59 +0x198
github.com/quic-go/quic-go.(*Transport).init.func1()
	github.com/quic-go/[email protected]/transport.go:266 +0x3b0
sync.(*Once).doSlow(0xb?, 0x1?)
	sync/once.go:74 +0x100
sync.(*Once).Do(...)
	sync/once.go:65
github.com/quic-go/quic-go.(*Transport).init(0x4000d18700, 0x78?)
	github.com/quic-go/[email protected]/transport.go:225 +0x58
github.com/quic-go/quic-go.(*Transport).dial(0x4000d18700, {0x5db44f6450, 0x40021bdea0}, {0x5db44ed5b0, 0x4000d7d5f0}, {0x0, 0x0}, 0x4000d109c0, 0x4000d1ab40, 0x1)
	github.com/quic-go/[email protected]/transport.go:212 +0x70
github.com/quic-go/quic-go.(*Transport).DialEarly(...)
	github.com/quic-go/[email protected]/transport.go:204
github.com/quic-go/quic-go.DialEarly({0x5db44f6450, 0x40021bdea0}, {0x5db44fc670?, 0x40000702f8?}, {0x5db44ed5b0, 0x4000d7d5f0}, 0x4000d109c0, 0x4000d1ab40)
	github.com/quic-go/[email protected]/client.go:95 +0xfc
github.com/xtls/xray-core/transport/internet/splithttp.createHTTPClient.func2({0x5db44f6450, 0x40021bdea0}, {0x5db3d00321?, 0x5bd7?}, 0x4000d109c0, 0x4000d1ab40)
	github.com/xtls/xray-core/transport/internet/splithttp/dialer.go:130 +0x1b4
github.com/quic-go/quic-go/http3.(*RoundTripper).dial(0x4000251e30, {0x5db44f6450, 0x40021bdea0}, {0x4003602db0, 0x10})
	github.com/quic-go/[email protected]/http3/roundtrip.go:312 +0x224
github.com/quic-go/quic-go/http3.(*RoundTripper).getClient.func1()
	github.com/quic-go/[email protected]/http3/roundtrip.go:249 +0x7c
created by github.com/quic-go/quic-go/http3.(*RoundTripper).getClient in goroutine 397
	github.com/quic-go/[email protected]/http3/roundtrip.go:246 +0x258

@LearZhou
Copy link

LearZhou commented Aug 2, 2024

我这边大致的测试中:prefer_new更友好

在alpn是h2时若使用prefer_reuse,反而会变慢,不如关闭mux

@RPRX
Copy link
Member

RPRX commented Aug 7, 2024

#3624#3643 合并后 rebase 一下我再看看吧

@RPRX
Copy link
Member

RPRX commented Aug 10, 2024

@ll11l1lIllIl1lll 可以 rebase 一下了,基于 main

@RPRX RPRX changed the title SpclithttpClient: a confusing mux SplitHTTP client: Add multiplex controller for H3 & H2 Aug 11, 2024
@RPRX
Copy link
Member

RPRX commented Aug 11, 2024

先改了下标题,晚点我比照着 #3560 (comment) 看下代码

@mmmray
Copy link
Collaborator

mmmray commented Aug 11, 2024

general comment: i think there are already too many things called mux. most are related to the v2ray universe. there is some dangerous overlap with http with "h2mux" in sing-box. this is a mux but the rfcs don't call it that. can it be called "connection pool settings" or something else?

@RPRX
Copy link
Member

RPRX commented Aug 16, 2024

首先 mode 不需要那么多别名,其次基础模式是二选一不是共存,prefer_reuse 就对应 concurrency,prefer_new 就对应另一个

好像没看到限制“一个连接最多被累计复用多少次”的代码

#3560 (comment) 下面说的 send rate 和 number of bytes sent/rcvd 等先不加,只是提一下,毕竟 PR 里也没有

@RPRX
Copy link
Member

RPRX commented Aug 16, 2024

general comment: i think there are already too many things called mux. most are related to the v2ray universe. there is some dangerous overlap with http with "h2mux" in sing-box. this is a mux but the rfcs don't call it that. can it be called "connection pool settings" or something else?

可以叫 XMC:Xray Multiplex Controller,因为这套机制完善后还要加给 Xray 的 Mux、gRPC、H2 等,虽然可能会把 H2 删掉

@RPRX
Copy link
Member

RPRX commented Aug 16, 2024

@Fangliding xPaddingBytes 的文档

@RPRX
Copy link
Member

RPRX commented Aug 16, 2024

@Fangliding 以及 "Cache-Control: no-store"

@mmmray
Copy link
Collaborator

mmmray commented Aug 16, 2024

it's too awkward to update the document when the release is not out. starting some updates here XTLS/Xray-docs-next#558

@Fangliding
Copy link
Member

Fangliding commented Aug 16, 2024

@Fangliding 以及 "Cache-Control: no-store"

#3652 的改动是默认的 没有config选项((((

@RPRX
Copy link
Member

RPRX commented Aug 23, 2024

To konsclufka:出来干活了

@RPRX
Copy link
Member

RPRX commented Aug 28, 2024

@mmmray 又要 rebase 了

@APT-ZERO
Copy link

Is this options for bypassing single connection speed limit of ISPs?
Is it possible to add same multi connection controller to gRPC?

@mmmray
Copy link
Collaborator

mmmray commented Aug 28, 2024

I'll take over this PR. I might add some more things if there is time. I hope to have it ready by sunday.

@APT-ZERO The answer is, yes can be added to grpc and a bunch of others, but it's not planned right now, and it can bypass speed limits like that, but I think if everybody uses it it will become meaningless (especially if you think of Irancell). That's all I'll say here. I suggest to go to github discussions or telegram for general discussion, I think this PR thread is already way too long and hard to navigate but i'm often the first one to complain about such things

@mmmray
Copy link
Collaborator

mmmray commented Sep 13, 2024

The PR has been updated, and my big "documentation" comment as well. I think it's good to review now.

The mux manager no longer works at round-tripper level, but actually muxes DialerClient, so basically the same design as originally done by @ll11l1lIllIl1lll (should definitely be credited). The main difference is that the mux manager is more generic and implements "concurrency" correctly.

@RPRX
Copy link
Member

RPRX commented Sep 15, 2024

改个名,既然 mux 是给原本没 mux 的东西加 mux,这个可以叫 demux,即 @mmmray 所说的 "un-mux"

MaxUses 改名 ConnectionReuseTimes

@mmmray
Copy link
Collaborator

mmmray commented Sep 15, 2024

done, but the "demux" is too strange and different from mux.cool settings.

In my mind "adding mux to config" means "configuring mux explicitly", not "adding mux". What about changing the default value of mux to concurrency=1 or just some value lower than infinity?

@RPRX
Copy link
Member

RPRX commented Sep 15, 2024

遇事不决 X,改名 xmux,不叫 HttpDemuxSplitHTTPMux,未来这套机制可以 port 给 gRPC,以及 Xray 现有的 mux

@mmmray
Copy link
Collaborator

mmmray commented Sep 15, 2024

bold proposal, mux settings tries to use the "transport-native" mux, then falls back to mux.cool if there is no such thing. prevent people from using mux.cool with h3, which is probably never a good idea (?)

@mmmray
Copy link
Collaborator

mmmray commented Sep 15, 2024

the config parameter is done, some structs are still named specific to splithttp, but it can be fixed in the PR where this is ported to other transports. fine, this was a bit too lazy

@RPRX
Copy link
Member

RPRX commented Sep 15, 2024

SplitHTTPMux->Xmux

@Fangliding
Copy link
Member

Fangliding commented Sep 15, 2024

依我看最好别把逻辑搞太复杂 check来check去的 直接在文档警告别启用mux.cool就是了 grpc也有类似的警告 或者这样多层套娃没准对打乱指纹也有用 split这个直接叫muxcontroller或者类似的东西(如标题) xmux这个名字留给以后真port到mux

@RPRX
Copy link
Member

RPRX commented Sep 15, 2024

In this case, the mux manager will open 8 connections first, then open more connections if the concurrency limit has been hit on all connections.

我明天再想一下这个,应该还要重命名

@RPRX
Copy link
Member

RPRX commented Sep 16, 2024

  • maxConnections
  • maxConcurrency
  • cMaxReuseTimes
  • cMaxLifeTimeMs

前两个只能二选一,不能出现突破预期的行为,比如 maxConnections 不能被突破,除非受到后两个限制要开一条新连接

@RPRX
Copy link
Member

RPRX commented Sep 16, 2024

前两个只能二选一,均有值则报错,均无值则不限制复用,但后两个限制仍然可以生效

@mmmray
Copy link
Collaborator

mmmray commented Sep 16, 2024

fineee, done. why restrict it though? it feels to me that this combination is fairly intuitive, although useless.

@RPRX
Copy link
Member

RPRX commented Sep 16, 2024

@mmmray json 忘改了,还有字段顺序调整一下

fineee, done. why restrict it though? it feels to me that this combination is fairly intuitive, although useless.

我看了下之前的代码也没有预连接(没有 minConnections),那么 maxConnections 实际上会变得 meaningless

@RPRX
Copy link
Member

RPRX commented Sep 16, 2024

关于默认值,现在相当于 maxConnections 默认为 1,其它均为 0

或者像 mux 一样改为默认由 maxConcurrency 主导,以及其它两项要不要有默认值,@mmmray 你怎么看

@mmmray
Copy link
Collaborator

mmmray commented Sep 16, 2024

Actually, the way the current code works, minConnections is more correct. It will open connections until the target has been reached. But not "pre": instead everytime a sub-connection is opened, a new connection is opened. ah, because it's not "pre" you want it not to be called "min". Ok

so i think 0 and 1 are mostly logically equivalent.

concurrency naming (without min/max) was mostly so that it can be equivalent to mux.cool settings.

I don't really know how one would dominate the other, since they are now mutually exclusive.

@mmmray
Copy link
Collaborator

mmmray commented Sep 16, 2024

Re-reading the comment, I think you're saying that maxConnections=1 default makes it impossible to set a config like "mux": {"maxConcurrency": 10}, but it's not true. You can set this kind of thing fine and it will behave exactly like mux.cool, because maxConnections can still be exceeded through setting maxConcurrency. The only thing I changed in the config is that it's not possible to explicitly set both parameters at the same time.

@RPRX
Copy link
Member

RPRX commented Sep 16, 2024

我觉得如果写了 maxConnections,按字面意思它就不该被突破,否则会造成困惑,anyway 我们先不设任何默认值吧

@mmmray
Copy link
Collaborator

mmmray commented Sep 16, 2024

Then i think the current state is good. maxConnections is broken if maxConcurrency is set to a value greater than 0. The user is not permitted to do that.

The other case is {"maxConnections": 10, "cMaxReuseTimes": 1}, then it's possible to exceed the concurrency limit temporarily, but I think that's ok.

@RPRX RPRX changed the title SplitHTTP client: Add multiplex controller for H3 & H2 SplitHTTP client: Add xmux (multiplex controller) for H3 & H2 Sep 16, 2024
@RPRX RPRX merged commit b1c6471 into XTLS:main Sep 16, 2024
36 checks passed
@RPRX
Copy link
Member

RPRX commented Sep 16, 2024

终于合了这个 PR,感谢各位的努力!

@RPRX
Copy link
Member

RPRX commented Sep 17, 2024

#3823 (comment)

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.

9 participants