-
Notifications
You must be signed in to change notification settings - Fork 2
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
Merge upstream v0.29.0 #25
base: main
Are you sure you want to change the base?
Conversation
This code is very closely based from the chrome QUIC C++ code (aka google/proto-quic), Thankfully this code base is very similar in it's layout to proto-quic making it a easy-ish port. For some of the missing holes, the Linux Kernel was looked at. It is worth pointing out that there are some core differences between this implementation and proto-quic, due to the fact that proto-quic has a lot more things passed down to it than this version has. To enable ease of merge and intergration I have decided to avoid doing that and instead (ab)use other bits of info that quic-go gives to it's congestion control code. In the end a bandwidth sampler needed to be implemented, and a good window filter was done as well. I *think* there already is another windowfilter in quic-go but it was not usable at the time. This commit does not impact quic-go in anyway since there is no intergration to it yet. This commit simply serves as the root commit to the BBR implementation.
You can now request to use BBR with the quic.Config{} section. Since the config section is not passed down to the function that makes the Y/N call on what congestion (there was only one until now) code to use, I passed down the bool only. Depending on how people feel about things might mean it's better to simply pass down the whole config section. But that feels to me excessive. Not holding any strong views though, up for doing it either way tbh.
... Otherwise BBR will never get setup correctly. Found using delve and a core dump of a production PID: ``` (dlv) print s *github.com/lucas-clemente/quic-go.session { handshakeDestConnID: github.com/lucas-clemente/quic-go/internal/protocol.ConnectionID len: 4, cap: 4, [83,254,26,34], origDestConnID: github.com/lucas-clemente/quic-go/internal/protocol.ConnectionID len: 0, cap: 0, nil, retrySrcConnID: *github.com/lucas-clemente/quic-go/internal/protocol.ConnectionID nil, srcConnIDLen: 4, perspective: PerspectiveServer (1), initialVersion: 0, version: VersionTLS (4278190109), config: *github.com/lucas-clemente/quic-go.Config { Versions: []github.com/lucas-clemente/quic-go/internal/protocol.VersionNumber len: 1, cap: 1, [VersionTLS (4278190109)], ConnectionIDLength: 4, HandshakeTimeout: net/http.http2prefaceTimeout (10000000000), MaxIdleTimeout: google.golang.org/grpc/credentials/google.tokenRequestTimeout (30000000000), AcceptToken: github.com/lucas-clemente/quic-go.glob..func1, TokenStore: github.com/lucas-clemente/quic-go.TokenStore nil, MaxReceiveStreamFlowControlWindow: 6291456, MaxReceiveConnectionFlowControlWindow: 15728640, MaxIncomingStreams: 1000, MaxIncomingUniStreams: 100, StatelessResetKey: []uint8 len: 0, cap: 0, nil, KeepAlive: false, UseBBR: false, ``` However in the root goroutine: ``` (dlv) print p *github.com/getlantern/http-proxy-lantern/v2.Proxy { TestingLocal: true, HTTPAddr: (unreadable could not read string at 0x4 due to error while reading spliced memory at 0x4: EOF), HTTPMultiplexAddr: "", HTTPUTPAddr: "\x00\x00\x00\x00", BordaReportInterval: 4, BordaSamplePercentage: 0, ... QUICUseBBR: true, OQUICAddr: "", ``` Me and forkner tracked this down in DM's, This should fix the issue
Forgot to take these out before submitting the "final" POC PR. Woops.
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.
I assume this is a straight-up merge from upstream, so if this compiles and does useful things, it looks good to me.
clicks Approve, runs away
It works but I don't think the workflows should have remained. I don't know why deletes aren't retained across merges and I get an empty file. |
No description provided.