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

Add KCP interface support #1142

Closed
wants to merge 2 commits into from
Closed

Add KCP interface support #1142

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 24, 2024

KCP is a fast and reliable ARQ protocol. KCP can provides faster connection in some environment.
I test this code, it works good.

@Vort
Copy link

Vort commented May 25, 2024

I wonder how you will build Windows 7 compatible binaries with this change.

@ghost
Copy link
Author

ghost commented May 25, 2024

I wonder how you will build Windows 7 compatible binaries with this change.

It's a problem that has to be faced, but Windows 7 Microsoft has stopped supporting it, and it's going to go.
kcp-go requires Go 1.21 or higher, it will indicate that Yggdrasil no longer supports Windows 7.
But this pull request can be delayed, until the author want to merge it.

@NikitaPuzyryov
Copy link

Fwiw @neilalexander had to say this in the Matrix room:

That's also a rather frightening list of new transitive dependencies...

@ghost
Copy link
Author

ghost commented May 25, 2024

I have a solution make Yggdrasil run on Windows 7. Use the old version of kcp-go. Add it to go.mod

replace github.com/xtaci/kcp-go/v5 => github.com/xtaci/kcp-go/v5 v5.6.1

This is the last compatible version of Go 1.20

@neilalexander
Copy link
Member

Thanks for raising the PR but I don't know that we'll accept it at this time. I'd also invite @Arceliar's opinion too.

We are concerned mostly about three things:

  1. KCP has quite a lot of tuneables that could potentially make it worse than TCP or QUIC congestion control — we don't want to expose those knobs to users but we don't know the best answers either;
  2. Our time is already somewhat limited and this is adding to our maintenance overhead when the vast majority of the network is using TLS/TCP;
  3. There's a lot of transitive dependencies in this library that suddenly we have to care about regarding security vulnerabilities & fixes.

If we were going to accept this, we'd need some convincing with concrete evidence (perhaps plotted on graphs) to prove that it performs well enough or better than the TLS/TCP and QUIC modes that we already have or that it satisfies some use-case that just isn't met today by our existing transports.

@ghost
Copy link
Author

ghost commented May 31, 2024

I have not enough machines to test the performance of KCP protocol in Yggdrasil. So this PR is closed.

@ghost ghost closed this May 31, 2024
@ghost ghost deleted the kcp branch May 31, 2024 11:31
This pull request was closed.
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