-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
improved datagram API #4471
Comments
Option 1: Stream-like APIWhy don't we have a similar problem with streams? Because a stream is an For datagrams, we could mirror this API: // NextDatagramWriter returns a new io.WriteCloser.
// The datagram is sent once it is closed.
NextDatagramWriter() io.WriteCloser This solves the prepend problem (problem 2) from above). Backpressure could be introduced by blocking the call to // NextDatagramWriter returns a new io.WriteCloser.
// If too many datagrams are queued for sending, this call blocks until it's possible to send more datagrams.
// The datagram is sent once it is closed.
NextDatagramWriter(context.Context) (io.WriteCloser, error) I'm not sure if I like this solution. It feels a bit clunky to use something that looks streamable (an I don't see how this helps with problem 3) though. The analogue, returning an Option 2: CallbacksThe receive side is easy: // SetDatagramHandler is called every time a DATAGRAM frame is received
SetDatagramHandler(func([]byte)) The different layers (HTTP/3 and CONNECT-UDP) could then wrap this callback, parse the first few bytes, and decide where to deliver it. This avoids all involvement of the Go runtime. This is likely very fast, however, this is also the biggest drawback: If any layer messes up, this directly blocks quic-go's run loop. Applications need to be very careful to not compromise transfer speeds, especially at higher bandwidths. The send side would look like this: // SetDatagramSender is called when a new datagram can be sent.
// If the callback returns nil, no datagram is sent.
// It is invalid to return a slice longer than maxLen.
SetDatagramSender(func(maxLen int) []byte)) This callback would be call a lot (once per packet). Again, it directly blocks quic-go's run loop. Optionally, the datagram collector could be run in a separate Go routine. The @chungthuang @joliveirinha @tanghaowillow @nanokatze @taoso Any thoughts on these API proposals? Any other suggestions? |
I don't like option 1 for the same reasons you mentioned, it appears really awkward and clunky, so I'll only focus on option 2. The reader side looks good, but as for sender, given that one of the intents was to avoid copying slices around, I think your suggested sender API runs somewhat counter to that? Perhaps it should be changed to be more like
Whatever layers could slice off the first few bytes for their own uses.
I'm kinda skeptical that implementing it like this is a good idea, it seems rather error-prone for the reasons you mention. I suspect that in the end things will have to be implemented in a way that tolerates the callback blocking, and quic-go would probably want to buffer a bunch of datagrams internally, kinda like the current push api works already. It's also not clear to me how, if the callbacks were supposed to be non-blocking, is the run loop supposed to know when the user got new datagrams available? Do we introduce a I'm also contemplating, should this API align with |
Thank you for thoughtful input @nanokatze!
You're totally right, I missed this. This won't work. We need to call a function to trigger the run loop to wake up. And I definitely don't like We could have an API like this: SendDatagram(context.Context, func(b []byte, maxLen int) []byte) error
Now calling a function with a callback is a bit clumsy, but at least it allows chaining without context switches and allocations. func SendDatagram(ctx context.Context, data []byte) error {
done := make(chan struct{})
if err := conn.SendDatagram(ctx, func(b []byte, maxLen int) []byte {
defer close(done)
// omitted: length check
return append(b, data...)
}); err != nil {
return err
}
<-done
}
Can you elaborate on this? I'm not sure I understand what you're proposing. |
What I was thinking of is pretty close to the callback-taking One small nit I have about the proposed API is that |
Hi @marten-seemann, I'm in favor of adding a context to the sender. It's likely that a datagram can be dropped if it's not sent within certain time frame. |
@chungthuang quic-go doesn't modify anything. As far as the QUIC layer is concerned, a datagram is just an opaque byte slice (the QUIC layer only does RFC 9221). It's only at the HTTP/3 layer that the flow identifier is defined (that's RFC 9297). |
quic-go has had QUIC Datagram support for a while, and the v0.43.0 release added HTTP Datagram support.
At every level, our API currently is:
While pretty idiomatic, this API causes a few problems:
SendDatagram
eventually blocks if it's not possible to send out datagrams quickly enough.ReceiveDatagram
causes a lot of context switches: The HTTP layer receives a datagram from connection's datagram queue, parses the varint, and then puts it into another datagram queue on the HTTP/3 layer.Problem 1) could easily be solved by adding a
context.Context
toSendDatagram
. Problem 2) and 3) seem to call for some more drastic API changes.The text was updated successfully, but these errors were encountered: