add a method to retrieve non-QUIC packets from the Transport

[marten-seemann]

Fixes #3929.

Tests still missing.
Resolution to https://mailarchive.ietf.org/arch/msg/quic/oR4kxGKY6mjtPC1CZegY1ED4beg/ still missing.

[codecov]

Codecov Report

Merging #3992 (89d3353) into master (501cc21) will decrease coverage by 0.02%.
Report is 2 commits behind head on master.
The diff coverage is 64.86%.

@@            Coverage Diff             @@
##           master    #3992      +/-   ##
==========================================
- Coverage   83.28%   83.26%   -0.02%     
==========================================
  Files         147      147              
  Lines       14806    14880      +74     
==========================================
+ Hits        12330    12389      +59     
- Misses       1978     1991      +13     
- Partials      498      500       +2     

Files Changed	Coverage Δ
internal/wire/header.go	81.41% <0.00%> (-0.83%)	⬇️
transport.go	67.45% <68.57%> (+0.15%)	⬆️

... and 3 files with indirect coverage changes

[AudriusButkevicius]

Don't see the point of this bool. Why wouldn't we always have the buffer allocated (based on size param somewhere) and just drop on overflow (or when sized to zero).

[marten-seemann]

Filled with packet, this would add 32*1500 = ~48 kB of wasted memory. I'd like to avoid that.

[AudriusButkevicius]

Not if the count is configurable and set to 0

[marten-seemann]

I was trying to avoid introducing yet more configuration API.

[AudriusButkevicius]

It's only configurable if you need it. It's zero by default, so doesn't seem like you need to provide a value?

[AudriusButkevicius]

Not if it's initializes at point the transport is constructed? The channel can be nil by default at which point we'd just drop the packets?

[AudriusButkevicius]

I'd still argue that this is not needed and could be done on nil'ness of the channel.

[lucas-clemente]

Variable assignment isn’t atomic in Go (iirc), so checking nil-ness and assigning from a different routine is technically undefined. You’d need to synchronize it with a mutex in that case. That’d work, but probably the explicit bool is a bit more readable?

[AudriusButkevicius]

I would still expose the responsibility of the channel creation/assignment to the builder of the transport type (before its used). Then you can use a nil check, and a user can control the buffer size (which is now quite small).

[marten-seemann]

I'm not convinced that's the best choice for the API here. I think I'm going to merge the PR as is, and we'll see how it works in practice. We can still change it later if we have a better idea.

[marten-seemann]

@bt90 I see you're blocked on this PR in syncthing. Have you tried out this branch yet? Does it work for your use case and allow you to retire the filter implementation?

[bt90]

@AudriusButkevicius

[AudriusButkevicius]

I don't think we are blocked. We just can't pick up the latest optimizations as we rely on this type of functionality which we are currently implementing with a separate package.

[calmh]

Maybe not blocked-blocked, but given that migrating to the new transport stuff is a refactor that doesn't fit cleanly into our existing way of doing it (since we can't write to the socket directly any more, nor read from the new transport), this would be welcome so that we don't need to rearchitect twice for the same change.