permit underlying conn to implement batch interface directly

[marten-seemann]

I reverted #3236 because I messed up the merge. This PR adds the changes.

[googlebot]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[marten-seemann]

@AudriusButkevicius Sorry for the confusion here, this is entirely my fault. Could you please consent as the GoogleBot is asking?

Regarding the release: This is more ad-hoc. Wasn't planning to cut a new release any time soon, unless there's a reason to do so. Would the batch-read something you'd be interested in having released soon?

[codecov]

Codecov Report

Merging #3237 (30d8691) into master (4e166bb) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

❗ Current head 30d8691 differs from pull request most recent head 3f3b8bd. Consider uploading reports for the commit 3f3b8bd to get more accurate results

@@            Coverage Diff             @@
##           master    #3237      +/-   ##
==========================================
- Coverage   85.46%   85.44%   -0.01%     
==========================================
  Files         133      133              
  Lines        9799     9804       +5     
==========================================
+ Hits         8374     8377       +3     
  Misses       1052     1052              
- Partials      373      375       +2     

Impacted Files	Coverage Δ
conn_oob.go	60.53% <50.00%> (-0.70%)	⬇️
internal/utils/rand.go	75.00% <0.00%> (+12.50%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e166bb...3f3b8bd. Read the comment docs.

[AudriusButkevicius]

@googlebot I consent.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[AudriusButkevicius]

Syncthing is stuck on an old version of quic-go as we are wary of breaking connectivity between versions like we did in the past by not reading quic-go release notes about compatability.

I guess now that RFC9000 is out, we should upgrade and keep rolling it forward, as I trust it should now never become incompatible.

Also, as part of that I'd like to pick up the batch reading code, not that I think the optimisations matter much, as we do our own reads and then fake a net.PacketConn anyway (to filter out stun packets via my pfilter package), but more so that the method quic-go uses to read packets would not change in the future release. Namely upgrading to the current latest release will give us RFC9000, which is great, but it still uses ReadUdpMsg (which we implement) but the next release would move to batch reads, meaning pfilter package would have to play catch up. I'd rather quic-go was released, I'd update pfilter to support ReadBatch, and forget about it (atleast until quic-go changes something again).

Ideally, quic-go would provide a way to read packets it does not recognize to be quic packets, that way I would not need pfilter package at all and could give quic-go a real net.UDPConn and trust that I somehow can recover stun packets that arrived on the same net.UDPConn, without really caring how quic-go implemente reads from net.UDPConn

[marten-seemann]

Syncthing is stuck on an old version of quic-go as we are wary of breaking connectivity between versions like we did in the past by not reading quic-go release notes about compatability.

That shouldn't be a problem if you're using tagged releases. As long as the QUIC (draft) version matches, it should be interoperable. Depending on how old your quic-go version is though, we might have dropped support for that particular draft version in a later release.

I guess now that RFC9000 is out, we should upgrade and keep rolling it forward, as I trust it should now never become incompatible

Sounds like a good idea. That version will be supported for a long time.

[AudriusButkevicius]

Depending on how old your quic-go version is though, we might have dropped support for that particular draft version in a later release.

I think that is exactly what happened, and our current version is 19.3, so I think we are going to have a breaking change regardless, as we move towards RFC9000.

Given the releases are ad-hoc, are you waiting for some specific PRs to land before you move forward? I guess I could upgrade us to the latest untagged commit, and assume nothing will break between that and the next future tag.

[marten-seemann]

Given the releases are ad-hoc, are you waiting for some specific PRs to land before you move forward?

Not really, although I'd probably want #3230 to go in (mind giving me a review, should be very straightforward?). Once that's in, I could cut a new release, there've been a couple of important changes since the last one.

[bt90]

Ideally, quic-go would provide a way to read packets it does not recognize to be quic packets, that way I would not need pfilter package at all and could give quic-go a real net.UDPConn and trust that I somehow can recover stun packets that arrived on the same net.UDPConn, without really caring how quic-go implemente reads from net.UDPConn

@marten-seemann does quic-go provide a mechanism like this? I'm not a go dev but unknownPacketHandler looks interesting.

[marten-seemann]

@marten-seemann does quic-go provide a mechanism like this? I'm not a go dev but unknownPacketHandler looks interesting.

It doesn't, mainly because of the implementation complexity: There are a lot of places where we drop packets, for various reasons (grep the code for DroppedPacket for a ~90% complete list), and we'd have to somehow hand back the packet from all of them.

[marten-seemann]

That said, if anyone comes up with a solution that's not super ugly I'd be willing to consider it.