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

improved datagram API #4471

Open
marten-seemann opened this issue Apr 28, 2024 · 2 comments
Open

improved datagram API #4471

marten-seemann opened this issue Apr 28, 2024 · 2 comments
Labels
Milestone

Comments

@marten-seemann
Copy link
Member

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:

SendDatagram([]byte) (error)
ReceiveDatagram(context.Context) ([]byte, error)

While pretty idiomatic, this API causes a few problems:

  1. SendDatagram eventually blocks if it's not possible to send out datagrams quickly enough.
  2. When sending datagrams, a common pattern is to prepend an identifier. HTTP datagrams prepend the Quarter Stream ID, and CONNECT-UDP prepends a flow identifier. While appending to a slice is very fast in Go (assuming sufficient slice capacity), it's not possible to prepend to a slice. Instead, the entire slice needs to be copied into a new slice.
  3. 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 to SendDatagram. Problem 2) and 3) seem to call for some more drastic API changes.

@marten-seemann
Copy link
Member Author

Option 1: Stream-like API

Why don't we have a similar problem with streams? Because a stream is an io.Writer, and every layer just writes there. It's possible to wrap an existing io.Writer, and insert data both in front of and after the Write call.

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 if datagrams can't be sent quickly enough on the QUIC connection:

// 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 io.Writer) to compose something that's inherently atomic, but at least it saves us from copying the payload over and over again.

I don't see how this helps with problem 3) though. The analogue, returning an io.Reader instead of a []byte, doesn't seem to give us any benefits.

Option 2: Callbacks

The 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 DatagramTooLargeError wouldn't be needed anymore, since the QUIC stack directly tells the application how much space there is left.


@chungthuang @joliveirinha @tanghaowillow @nanokatze @taoso Any thoughts on these API proposals? Any other suggestions?

@marten-seemann marten-seemann added this to the v0.44 milestone Apr 28, 2024
@nanokatze
Copy link
Contributor

nanokatze commented Apr 28, 2024

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 io.ReaderFrom:

SetDatagramSender(func(p []byte) int)

Whatever layers could slice off the first few bytes for their own uses. len(p) indicates the maximum datagram size, the returned int is the size of the datagram to be sent. I'm not too sure how we should discriminate between the user not wanting to send and wanting to send a zero-sized datagram with this API. I guess 0 and a special sentinel value of -1 could be made to mean "do not send" and "send zero-sized datagram" respectively, as I suspect zero-sized datagrams are not that common of a case.

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.

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 WakeRunLoop() of some sort? Letting the callback block would naturally address that: run loop should wake up when the callback returns.

I'm also contemplating, should this API align with ReaderFrom/WriterTo better? So that instead of methods to set callbacks, we introduce ReceiveDatagrams and SendDatagrams counterparts take the respective callbacks and block the caller until the callbacks return some value to signal that they don't want to be called anymore (e.g. an error that then gets propagated to the caller.) I'm not really sure how useful this direction would be but for me it does feel compelling to not diverge from the familiar interfaces where necessity isn't obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants