Make quic-go more STUN friendly about quic-go HOT 18 CLOSED

No rush on my part. With this merged we can do our change with the dependency pointing at main, and we can just point it at a real quic-go release before we release.

from quic-go.

Thanks for all the work!

from quic-go.

Related discussion: #3727 (comment). Let's continue the discussion here, now that we have the new Transport.

Pinging @AudriusButkevicius @calmh @ydnar @zllovesuki @tobyxdd, who were all involved in that discussion.

The QUIC packet header was designed to make it possible to demultiplex with common protocols: quicwg/base-drafts#426. We introduced the "QUIC bit" (0x40), which is set to 1 in both Short Header and Long Header packets: quicwg/base-drafts#426. While this is not a version-independent property, it holds for both QUIC v1 and QUIC v2.

We could make it possible for users of a Transport to get access to packets that don't have the QUIC bit set. Would that solve your use case?

I can see two ways this could be implemented:

1. Add a Transport.ReadNonQUICPacketFrom(p []byte) (int, net.Addr, error) method (suggestions for a better name would be appreciated). Internally, this method would read packets from a channel. After the first call to this function, the listen Go routine would add non-QUIC packets to this channel (so we only create the channel if know that there's actually a consumer), dropping them if the channel overflows.
2. Add a callback to the the Transport: HandleNonQUICPacket(b []byte, net.Addr). If set, the listen Go routine would call this callback with non-QUIC packets. It's the applications responsibility to process the packet quickly and not block the run loop.

Tradeoffs:

• (1) is arguably the more idiomatic API
• (1) requires an additional copy into the user-provided buffer
• (2) doesn't require a copy, but wouldn't allow us to reuse the buffer, effectively leading to an extra allocation for every non-QUIC packet

Given these tradeoffs, I'm leaning towards (1). Thoughts? Anything else we should consider here?

from quic-go.

What about providing a net.PacketConn, that also supports writing etc, that writes to the real transport, and a config option in the transport to adjust the buffer size?

t := Transport {
   NonQuicPacketBufferSize: 1 << 20,
}
conn := t.GetNonQicPacketConn()

Both of the proposed APIs still require you to keep a handle on the write side of things, so this would solve that.

from quic-go.

Would this allow us to drop the OptimizeConn function? One thing you wouldn't be able to do with the new API is have a way to only inject packets (but not read packets), but maybe we can live with that?

from quic-go.

What if handling non-QUIC packets was the responsibility of the caller, via their own net.PacketConn implementation?

This package could provide a helper func IsQUIC(b []byte) to ease that process.

from quic-go.

Or this package could provide a PacketConn type that wraps/implements net.PacketConn plus ReadQUICFrom() that Transport would use if present.

from quic-go.

@ydnar We'd like to be in control of the underlying UDP socket. We already do that to read the ECN bits and the packet info. At some point, quic-go will add support for GRO (#3838). As soon as we do that, every call to Read might return giant packets that we first need to chop up. This will introduce some significant complexity, but we've seen with GSO that batch handling can lead to a dramatic increase in performance.

from quic-go.

If we only look at the STUN use case, both approaches would work. The question is if someone would want to demultiplex a protocol with more traffic. The first proposal might drop packets if the channel overflows.

You also mentioned that the second proposal suffers from more allocations because quic-go can't reuse the buffer. Wouldn't it be better to split the burden of buffer reuse and allow the non-QUIC side to provide its own buffer?

from quic-go.

You also mentioned that the second proposal suffers from more allocations because quic-go can't reuse the buffer. Wouldn't it be better to split the burden of buffer reuse and allow the non-QUIC side to provide its own buffer?

How would the API look like?

from quic-go.

I've no real insight on the internals of quic-go and I'm not a go developer but you're reusing buffers by pooling them? Can't we use a dedicated pool or a second pooled buffer for non-QUIC consumers and transfer the content to that buffer? At least if the intention is to avoid concurrent access.

from quic-go.

I don't really want to expose quic-go's buffer pool to the outside world. If we want to make pooling possible, the user needs to be able to pass the buffer into quic-go.

The first proposal might drop packets if the channel overflows.

To be honest, this is probably not as bad. There's a lot of places in the networking stack where packets are dropped when a queue overflows, this would just be one more.

from quic-go.

What about providing a net.PacketConn, that also supports writing etc, that writes to the real transport, and a config option in the transport to adjust the buffer size?

t := Transport {

   NonQuicPacketBufferSize: 1 << 20,

}

conn := t.GetNonQicPacketConn()

This is probably the most sane proposal. I'd bikeshed it to (*Transport).UnknownPacketConn or (*Transport).OtherPacketConn, but it makes sense for multiplexed reads/writes.

If a caller grabs this PacketConn, would Transport implicitly start a server?

from quic-go.

If a caller grabs this PacketConn, would Transport implicitly start a server?

There's no need to start a server, all we need to do is start listening on the connection.

I'm wondering if we can find an API that fixed the problem I described in #3911 (comment) (the last point about the WriteTo method).

from quic-go.

#3992 is a draft implementation of (1) without any configuration option for the buffer size.

from quic-go.

@marten-seemann any ETA for the next release?

from quic-go.

Some time next week probably. The milestone is almost done: https://github.com/quic-go/quic-go/milestone/20

Any reason to release early? Are you waiting for this release?

from quic-go.

I think the syncthing maintainers are in no hurry, but it wouldn't hurt to have a release that works with go 1.21 and avoids the minefield of having to adapt the current packet filtering to the GSO changes.

(disclaimer: i'm not one of the maintainers)

from quic-go.