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

XHTTP Client: Race Dialer #4320

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

XHTTP Client: Race Dialer #4320

wants to merge 2 commits into from

Conversation

rPDmYQ
Copy link
Contributor

@rPDmYQ rPDmYQ commented Jan 23, 2025

Add Race dialer for XHTTP:

  • Enable by specifying both h3 and h2 in ALPN.
    • QUIC connection uses only h3 as ALPN.
    • TCP TLS connection uses everything else as ALPN.
  • Try connecting to server using both h3 and h2 connection. The first connection that succeed dialing is used.
  • If there's already established connection, it will be reused.
  • Chrome-like try logic:
    • Delay initial h2 attempt to prefer h3.
    • H3 gets broken mark and won't be attempted for a while upon failure.

@Fangliding
Copy link
Member

看起来不错 不过有点点小问题

对不同的host进行了等待防止多次race 这很好 但是xhttp中的单个client不会有其他目标 所以对不同的host等待是没有必要的
又还有 这个等待压根不会生效 因为这个检查是client级别的 xhttp在连接的初始化阶段会创建多个client 相互之间不共享这个信息 你可能需要创建一个全局变量存放race结果

这样复杂的功能最好有个test(好像有点难写)

最后有俩 type switch 虽然知道是安全的但还是写了panic 一个小建议改成 fmt.Sprintf("unreachable: unexpected response type %T", value) 之类的输出具体类型

@rPDmYQ
Copy link
Contributor Author

rPDmYQ commented Jan 24, 2025

xhttp中的单个client不会有其他目标

You are right that each raceTransport only connects to one endpoint and there can only have one inflight race per transport.

这个等待压根不会生效

The race wait does happen for stream-up and packet-up if there's no separated downloadSettings. Requests of both directions are handled by the one http client (therefore transport).

@Fangliding
Copy link
Member

我的表达可能有点问题 stream up的两个请求和packet up的多个请求确实会在一个客户端 这个等待部分有用 但是假如有多个请求同时进入核心会涉及多个client 这个仍会多次race 不过如果觉得这是正常倒也说得过去

@rPDmYQ
Copy link
Contributor Author

rPDmYQ commented Jan 24, 2025

Multiple clients will create multiple connections. Since they don't share connection, we should let them start race together - otherwise we would waste 1-RTT for all but one connections just to know the race winner.

And Chrome's default parameters are really trying to avoid real racing. H3 handshake uses only 1-RTT, so > 300ms means

  • this is a ready crappy server / crappy route
  • the network is dropping UDP packets

The h3EndpointCatalog doing statistics on H3 connections is global.

@RPRX
Copy link
Member

RPRX commented Jan 25, 2025

感谢 PR,为了防止损坏现有功能,我倾向于这个 PR 放下个版本之后

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.

3 participants