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

Bump quic-go to v0.15.5 #3732

Merged
merged 10 commits into from
Apr 28, 2020
Merged

Bump quic-go to v0.15.5 #3732

merged 10 commits into from
Apr 28, 2020

Conversation

matzf
Copy link
Contributor

@matzf matzf commented Apr 28, 2020

Bump quic go to latest release v0.15.5.

Update to changes in quic-go API:

  • Provide context in various API calls.
  • session.Close() is gone, now there is only session.CloseWithError(errorNoError, "")
    Handle this no-error error (by ignoring it) in pingpong.

Co-authored-by: FR4NK-W wirzf@inf.ethz.ch
Replaces #3543


This change is Reviewable

@lukedirtwalker lukedirtwalker self-requested a review April 28, 2020 10:32
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)


go/lib/infra/rpc/rpc.go, line 41 at r1 (raw file):

	CtxTimedOutError = iota + 1

	errorNoError quic.ErrorCode = 0x100

is that just a random error code that you selected, or how did you select this?
I wonder if we should also use 0x0 as quic internally uses: https://godoc.org/github.com/lucas-clemente/quic-go/internal/qerr#ErrorCode ?

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)

Copy link
Contributor Author

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)


go/lib/infra/rpc/rpc.go, line 41 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

is that just a random error code that you selected, or how did you select this?
I wonder if we should also use 0x0 as quic internally uses: https://godoc.org/github.com/lucas-clemente/quic-go/internal/qerr#ErrorCode ?

My understanding is that what we set here is an application error code, while this enumeration in quic-go/internal/qerr refers to transport error codes.
As far as I understand, these values are completely arbitrary and can be chosen by the application (https://datatracker.ietf.org/doc/draft-ietf-quic-transport/?include_text=1, section 20.1). I picked 0x100 because that's what the http3 client uses (https://github.com/lucas-clemente/quic-go/blob/master/http3/error_codes.go).

matzf added 2 commits April 28, 2020 15:13
In gazelle-v0.19.1:
> "The list of standard packages is correctly updated for Go 1.13."

This fixes the issue:
> gazelle: finding module path for import crypto/ed25519.
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lukedirtwalker lukedirtwalker merged commit 2157b12 into scionproto:master Apr 28, 2020
@oncilla oncilla mentioned this pull request May 8, 2020
oncilla added a commit to oncilla/scion that referenced this pull request May 8, 2020
Add RPC retry with exponential back-off in case of quic `server busy`
error.
Reduce synchronous part of accept loop.

With the bump to quic-go v0.15.5 (scionproto#3732) the quic library now enforces a
max accept queue length. With our approach of every RPC is a new quic
connection, and some work being done synchronously in the accept loop
this bites on CI heavily.

With the exponential back-off and immediately spawning a go routine, we
can improve failure rate significantly.

But this should not be the final solution. Applications need to be
able to deal with requests failing. Currently, we only do one shot
testing, which might be too strict.

For investigation results, see the buildkite builds associated with
the draft PR scionproto#3750.
oncilla added a commit that referenced this pull request May 11, 2020
Add RPC retry with exponential back-off in case of quic `server busy`
error.
Reduce synchronous part of accept loop.

With the bump to quic-go v0.15.5 (#3732) the quic library now enforces a
max accept queue length. With our approach of every RPC is a new quic
connection, and some work being done synchronously in the accept loop
this bites on CI heavily.

With the exponential back-off and immediately spawning a go routine, we
can improve failure rate significantly.

But this should not be the final solution. Applications need to be
able to deal with requests failing. Currently, we only do one shot
testing, which might be too strict.

For investigation results, see the buildkite builds associated with
the draft PR #3750.
@matzf matzf deleted the bump-quic-go branch August 19, 2020 06:49
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.

2 participants