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

Merge upstream v0.29.0 #25

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

Merge upstream v0.29.0 #25

wants to merge 10 commits into from

Conversation

anacrolix
Copy link

No description provided.

forkner and others added 10 commits April 28, 2022 11:40
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.
Copy link

@Crosse Crosse left a 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

@anacrolix
Copy link
Author

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.

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.

4 participants