-
Notifications
You must be signed in to change notification settings - Fork 191
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
feat: support bandwidth configuration #645
base: main
Are you sure you want to change the base?
Conversation
提问,
|
否,Hysteria2 协议会在握手时协商双方带宽。我们实现的行为是:如果服务端和客户端任意一方不提供带宽或者服务端选择忽略客户端带宽,那么它将会选择启用 BBR。只有当双方均设置了带宽的前提下才会取双方最小的带宽进行通信,具体流程在 上游 Hysteria 文档中有描述。
实际上确实有可能存在「服务端和客户端都提供了带宽参数,但是实际上不支持 Brutal」的情况,且根据协议这似乎是一个 Unspecified Behavior。但是由于我们的协商行为和官方客户端一致,所以当这种这种情况存在时,则该服务端将和一般通过官方客户端的互操作性也会难以保证,所以我们大概可以忽略这种情况。 |
@mnixry 我不是一个 hy2 用户,但你解释的非常清楚,且对于我了解 PR 相关的风险非常重要,多谢解答 |
@mnixry 麻烦在 pr 描述中也加入配置文件的示例,谢谢 |
done |
474f55c
to
f868f06
Compare
|
||
# The maximum bandwidth for accessing the Internet. It is useful for some specific protocols (e.g., Hysteria2), | ||
# which will perform better with bandwith information provided. The unit is **byte** per second. | ||
bandwidth_max_tx: 26214400 # 200Mbps == 25MB/s == 26214400 B/s uplink |
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.
Do not easily add global parameters. Besides, there are not many protocols that support bandwidth settings (as far as I know). It is sufficient to design a bandwidth query parameter in the URI schema.
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.
Do not easily add global parameters. Besides, there are not many protocols that support bandwidth settings (as far as I know). It is sufficient to design a bandwidth query parameter in the URI schema.
@douglarek Thanks for the review.
The upstream PR also includes supports for importing maxTx
and maxRx
from the URI scheme, and these have higher priority over global configurations.
There are several reasons to consider adding bandwidth settings as a global option:
- When using subscriptions with URIs provided by external sources, you might not have control over the URI parameters they include.
- In principle, a URI should contain only server-related details, ensuring it can be shared across devices without modifications. Bandwidth preferences, on the other hand, are client-specific and shouldn’t be universally shared.
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.
Do not easily add global parameters. Besides, there are not many protocols that support bandwidth settings (as far as I know). It is sufficient to design a bandwidth query parameter in the URI schema.
@douglarek Thanks for the review.
The upstream PR also includes supports for importing
maxTx
andmaxRx
from the URI scheme, and these have higher priority over global configurations.There are several reasons to consider adding bandwidth settings as a global option:
1. When using subscriptions with URIs provided by external sources, you might not have control over the URI parameters they include. 2. In principle, a URI should contain only server-related details, ensuring it can be shared across devices without modifications. Bandwidth preferences, on the other hand, are client-specific and shouldn’t be universally shared.
Regarding what you mentioned in point 1, I think the idea is good, but its actual implementation may not be perfect. If you have two subscriptions and their bandwidths are different, this global setting would not be useful. Regarding 2, this is actually intentional and cannot be considered a general rule. This is essentially a major pitfall set by the protocol designer.
Background
ref: daeuniverse/outbound#21
Checklist
Full Changelogs
Implement bandwidth config parsing in dae:
Improve handling for
insecure
parameter in hysteria2 URI parsing.Issue Reference
Closes #[issue number]
Test Result