better control over keep-alives

[marten-seemann]

Currently our configuration flag for keep-alives is an all-or-nothing: it's a bool, so keep-alives are either on or off.

We could instead introduce a callback instead that lets the application decide if the connection should be kept alive.

quic.Config{
     KeepAlive func() bool
}

Open question: when exactly should this callback be called?

[raskyld]

Hi!

First things first, thanks a lot for your work on this library :)

---

Here is my use-case of quic-go, if you want some context 😄

I am implementing a PoC virtual network fabric to mesh Go processes together and provide an equivalent of chan built on top of quic.Stream which I call Flow, I built an adaptor to run https://github.com/hashicorp/memberlist on top of quic.Transport to provide a gossip-based decentralized naming service. Instead of plain UDP, I send inter-node datagrams using quic.Connection's SendDatagram() if there is an already active connection to the target node.

This is working pretty well and I plan to open-source it soon ™
Atm, I am mostly doing clean-up, improvements and try to run it at higher scale to see how it behave in real-life scenarios.

---

One area I want to improve is keeping alive the connection only if there are Flows active on the quic.Connection, that is, if a connection is just open as part of the gossip protocol anti-entropy health check but there is no active stream between the two nodes, there is no reason to keep it alive.

Having control over the main loop keep-alive is exactly what I need, I will likely try to contribute a PR for this issue, I just want to discuss the open question:

What do you think about making this function:

quic-go/connection.go

Line 708 in 4ab8ec4

if s.config.KeepAlivePeriod == 0 || s.keepAlivePingSent {

Return a zero time if the callback return false?

Thanks!

[marten-seemann]

@raskyld Your use case totally makes sense, and I'd love to better support it. I'd like to get the API right though this time.

Currently, we have a Config.KeepAlivePeriod, which defines when a keep-alive PING is sent. Well, kind of, we might send it earlier, depending on the negotiated idle timeout (see section 10.1 of RFC 9000).

Now we could add the KeepAlive callback as I suggested earlier, but now we have two keep-alive-related fields in the Config. Not the end of the world, but would be nicer to only have one.

Any opinions?

[raskyld]

Sorry for the late answer! GH notification got lost in my mails 😬

I understand your concern, if we want to keep the keep (pun intended) alive related config minimalistic in quic.Config, maybe we could instead extend the API of quic.Connection to allow users to raise or lower a flag KeepAlive(bool).

The advantage of this approach versus calling a callback in each iteration of the main loop is that this could be backed by a lock-less atomic flag which would avoid introducing a too liberal user-defined code which could impact the main loop performance.

Indeed, the cons is that is extend the API surface of Connection.

WDYT?

[marten-seemann]

That works, but it still has from the same amount of API surface, just now spread across Config.KeepAlivePeriod and Connection.KeepAlive.

[raskyld]

What if we instead only have a Connection.KeepAlive(FLAG), where FLAG could be:

• Pessimistic (Send every 3*PTO)
• Optimistic (Send every negociatedMaxIdleTO / 2)
• BestEffort(period time.Duration) (Try to use user provided period but fall back to Pessimistic or Optimistic if out of bound).
• NoKeepAlive (Turn off Keep Alive)

In Rust that would be easy to do with an Enum but in Go since we need BestEffort to come with a tuple, we likely would need something like:

type KeepAliveKind uint8

var (
    Pessimistic KeepAliveKind = iota
    Optimistic
    BestEffort
    NoKeepAlive
)

type KeepAliveParam struct {
    kind KeepAliveKind
    // only checked-in when kind == BestEffort
    period time.Duration
}

type Connection interface {
    // ...
    KeepAlive(param KeepAliveParam) // don't think we need to pass by pointer, the struct is small enough to be kept on the stack
    // ...
}

WDYT?

[raskyld]

Hey, FYI I just released my project under Apache 2.0 😄

https://github.com/raskyld/grinta

If you wanna understand how I use / if I misuse go-quic feel free to have a 👀 !