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

fix: update to lucas-clemente/quic-go v0.23.0 #448

Closed
wants to merge 4 commits into from
Closed

Conversation

bassosimone
Copy link
Contributor

@bassosimone bassosimone commented Aug 23, 2021

Checklist

Location of the issue tracker: https://github.com/ooni/probe

Description

This pull request updates to lucas-clemente/quic-go v0.23.0. Updating leads to several CI runtime errors (see, e.g., this failed run: #441 (comment)). To avoid these errors, when we are listening for QUIC, we need to return a type that is both a quic.OOBCapablePacketConn and net.Conn. Otherwise, the underlying golang.org/x/net/ipv4.NewPacketConn function is going to panic because we are using a connection that cannot be converted to a net.Conn. Therefore, this PR introduces a new internal/quicx.UDPLikeConn interface that implements both quic.OOBCapablePacketConn and net.Conn. See ooni/probe#1739.

It is also worth mentioning that this problem arose in #441, which contained backports from the stable branch. Since authoring these changes, I realised it's quite pointless to apply more changes in the stable branch, so I dropped f0d05c5 from the patchset.

@bassosimone bassosimone requested a review from hellais as a code owner August 23, 2021 12:19
// This code introduces the UDPLikeConn, whose documentation explain
// why we need to introduce this new type. We could not put this
// code inside an existing package because it's used (as of 20 Aug 2021)
// by the netxlite package as well as by the netx package.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
// by the netxlite package as well as by the netx package.
// by the netxlite package as well as by the netxmocks package.

@bassosimone
Copy link
Contributor Author

When running urlgetter using miniooni with HTTP3Enabled=true, I see this error:

2021/08/23 14:23:39 connection doesn't allow setting of receive buffer size. Not a *net.UDPConn?. See https://github.com/lucas-clemente/quic-go/wiki/UDP-Receive-Buffer-Size for details.

It's important to note that we don't see any read_from entry as well.

From the above data we can deduce that:

  1. our test suite with -short does not include enough checks to notice that read_from is missing
  2. reading quic-go code, it seems ~obvious we need to implement SetReadBuffer

I am going to open an issue for the former problem and to fix the latter as part of this pull request.

@bassosimone
Copy link
Contributor Author

Even after d587f5a, we are still without the read_from information in the measurement results. So, it seems this is another issue.

@bassosimone bassosimone added the wontfix This will not be worked on label Aug 23, 2021
@bassosimone
Copy link
Contributor Author

Superseded by ooni/probe#1754, for which a PR will come soon.

@bassosimone bassosimone deleted the issue/1739 branch August 23, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant