Repository: quic-go/quic-go

Issue:
Connection Migration API
This is a proposal for adding a Connection Migration API.

The `Config` gains a new field that controls if _the peer_ (not the endpoint itself) is allowed to initiate connection migration:
```go
type Config struct {
    // …
    // AllowConnectionMigration controls if the peer is allowed to initiate connection migration.
    AllowConnectionMigration bool
}
```

The state can be checked on the `ConnectionState`. See below for more explanation on `Path`.
```go
type ConnectionState struct {
     // …
     // ConnectionMigrationDisabled says if it’s possible to migrate this connection to a new interface.
     // The peer has the option to disable migration by sending the disable_active_migration transport parameter.
     ConnectionMigrationDisabled bool
     // Path is the network path that’s currently in use.
     Path *Path
}
```

On the client side, migration is initiated by calling `ProbePath` on the `Connection`:
```go
// PathStatus is the status of a path.
type PathStatus int

const (
     // PathStatusProbing means that the path is being probed (i.e. a PATH_CHALLENGE frame has been sent).
     PathStatusProbing PathStatus = iota
     // PathStatusTimeout means that path probing ran into a timeout,
     // or that a previously successfully probed path was abandoned.
     PathStatusTimeout
     // PathStatusProbeSuccess means that path probing succeeded. It is now possible to switch to this path.
     PathStatusProbeSuccess
     // PathStatusActive means that this is the path that’s used to send QUIC packets
     PathStatusActive
)

// Path is a network path.
type Path struct {
     // Status is the status of this path.
     Status PathStatus
     // Notify is a channel (cap 1) that a new value is added to every time the status changes.
     // Note that the status can change even after path validation succeeded (e.g. because a path times out).
     Notify <-chan struct{}
     // might add some path characteristics (RTT, MTU, loss rate, etc.) here later
}

// Commit switches the QUIC connection to this path.
// It immediately stops sending on the old path, and sends on this new path.
// It’s only valid to call Use if the status is ProbeStatusProbeSuccess.
func (*Path) Commit() error

// Abandon abandons a path.
// It is not possible to abandon the path that’s currently active.
// This stops the sending of keep-alive packets, eventually leading to a timeout of the path.
func (*Path) Abandon() error

// ProbePath probes a new path.
// This triggers send a packet containing a PATH_CHALLENGE frame and receiving a PATH_RESPONSE frame.
// Therefore, probing takes at least one RTT.
// Note that the RTT of the new path might be different from the old path’s RTT.
// It is only valid
// 1. on the client side, and
// 2. if the peer hasn’t disabled connection migration.
// Additionally, this function will return an error if there’s no spare Connection ID available.
// Once successfully probed, the path will be kept alive by regularly sending keep-alive packets.
func (*Connection) ProbePath(*Transport) (Path, error)
```

Why does `ProbePath` take a `Transport`, and not an IP address or a `net.PacketConn`? This is because we need to be able to receive packets sent on the new path, i.e. we need to demultiplex them from other packets that might be received on the same interface. This is _exactly_ what a `Transport` does.
Internally, `ProbePath` adds _all_ the connection IDs that we issued to the peer to the new `Transport` as well. Since in RFC 9000, connection IDs are not bound to paths, we can’t know which CID the peer will use on the new path. As a slight optimization, we might be able to omit CIDs that are already used on other paths.

Hints from issue comments:
@None: come on  I love this@masx200: https://github.com/quic-go/quic-go/issues/4443@yinbao2000: In the proposed API, does the client must explicitly initiate `PathProbe` and then `Commit`?  If that is the case, then the connection migration is not transparent to the application. It will also come with performance cost because it's driven by how well the application is developed to make use of the feature. How about adding an option in API where client can 1) specify the path options 2) set the KPIs of migration (similar to the characteristic in `notify`. Example: switch to another path when active path pkt loss is over 20%)?  These settings are done ahead of time and the migration happens later without involvement of the application. I know that this may turn connection migration into a traffic engineering middleware that also have to do the jobs of preventing pingpong of paths. But leaving that job to the application client is probably worse?@marten-seemann: > If that is the case, then the connection migration is not transparent to the application. 

That is **explicitly** not the intention. Connection migration is an advanced feature, and will require fine tuning depending on the use case.@stv0g: > That is explicitly not the intention. Connection migration is an advanced feature, and will require fine tuning depending on the use case.

I agree. We need this to be explicit in order to interoperate NAT-traversal / ICE.

There are several proposed RFCs which are somewhat based on QUICs connection migrations:

* https://datatracker.ietf.org/doc/html/draft-perkins-quic-p2p-mux-00
* https://datatracker.ietf.org/doc/html/draft-thatcher-p2p-quic-00
* https://datatracker.ietf.org/doc/html/draft-seemann-quic-nat-traversal-02@fortuna: Thanks @marten-seemann for advancing this!

When designing an API, I find it helpful to not only describe the APIs, but also to show how they would be used in some common cases, to validate it.

For instance how would an app that uses an `http.Client` that is capable of migrating connections look like?

From the app perspective, the logic can be in the http3.Transport. But how would that work?

I'm trying to figure that out, but here is my understanding of what needs to be done on network change:

1. Create a new connection on the new network
   1. Create a new `net.PacketConn` on the new network.
   1. Create a new `quic.Transport` with the new packet conn.
   1. Call `Transport.DialEarly` to get a new `quic.EarlyConection`
1. Call `Connection.ProbePath` on the new early connection
1. If successful, replace the early connection that the HTTP client is using somehow. We need to do this for all active connections, we will need to intercept and track all HTTP client connections somehow.

It may be helpful to have an Early Connection type that supports replacing the underlying `EarlyConnection`. Or can we just replace the `PacketConn`?

We could also consider adding a `quic.Migrate(quic.Transport, net.PacketConn)`

Does that make sense? i'm still new to QUIC and trying to figure it out.
@fortuna: I'm a bit confused. Is this proposal for the client or server API?

I'm reading the RFC, and according to [Initiating Connection Migration](https://datatracker.ietf.org/doc/html/rfc9000#name-initiating-connection-migra), the client initiates the migration by simply sending packets from the new location.

That means we could simply replace the `net.PacketConn` that the `quic.Transport` and `quic.Connection` are using.
No need for probing, since that's the server's responsibility. Of course, the client code needs to still respond to the probes.

In that case, would it make sense to simply have a `quic.Transport.Migrate(net.PacketConn)` API?
@marten-seemann: > It may be helpful to have an Early Connection type that supports replacing the underlying `EarlyConnection`.

I don't understand. The whole point of connection migration is to keep the connection state.



> I'm reading the RFC, and according to [Initiating Connection Migration](https://datatracker.ietf.org/doc/html/rfc9000#name-initiating-connection-migra), the client initiates the migration by simply sending packets from the new location.
>
> That means we could simply replace the net.PacketConn that the quic.Transport and quic.Connection are using.
No need for probing, since that's the server's responsibility. Of course, the client code needs to still respond to the probes.

This is not correct. The client has to probe the new path before using it. Even if it was not mandated by the RFC, it wouldn't be a smart move to give up a working path for a new path that might or might not work at all.

> For instance how would an app that uses an `http.Client` that is capable of migrating connections look like?

That's a separate question. HTTP/3 is an application and the code is just part of this repo for reasons of convenience (see #4674). This issue is dealing with a transport-layer API. If / how this we'll expose this at the HTTP/3 layer remains to be seen.

Solution patch:
diff --git a/path_manager.go b/path_manager.go
index 0a5b9cf9..aac8103d 100644
--- a/path_manager.go
+++ b/path_manager.go
@@ -12,6 +12,8 @@ import (
 
 type pathID int64
 
+const invalidPathID pathID = -1
+
 const maxPaths = 3
 
 type path struct {
diff --git a/path_manager_outgoing.go b/path_manager_outgoing.go
index 95ec140c..595eda23 100644
--- a/path_manager_outgoing.go
+++ b/path_manager_outgoing.go
@@ -14,8 +14,12 @@ import (
 	"github.com/quic-go/quic-go/internal/wire"
 )
 
-// ErrPathNotValidated is returned when trying to use a path before path probing has completed.
-var ErrPathNotValidated = errors.New("path not yet validated")
+var (
+	// ErrPathClosed is returned when trying to switch to a path that has been closed.
+	ErrPathClosed = errors.New("path closed")
+	// ErrPathNotValidated is returned when trying to use a path before path probing has completed.
+	ErrPathNotValidated = errors.New("path not yet validated")
+)
 
 var errPathDoesNotExist = errors.New("path does not exist")
 
@@ -28,6 +32,7 @@ type Path struct {
 
 	enablePath func()
 	validated  atomic.Bool
+	abandon    chan struct{}
 }
 
 func (p *Path) Probe(ctx context.Context) error {
@@ -47,6 +52,8 @@ func (p *Path) Probe(ctx context.Context) error {
 		case <-timerChan:
 			p.pathManager.enqueueProbe(p)
 		case <-path.ProbeSent():
+		case <-p.abandon:
+			return ErrPathClosed
 		}
 
 		if timer != nil {
@@ -62,11 +69,37 @@ func (p *Path) Probe(ctx context.Context) error {
 // It immediately stops sending on the old path, and sends on this new path.
 func (p *Path) Switch() error {
 	if err := p.pathManager.switchToPath(p.id); err != nil {
-		if errors.Is(err, errPathDoesNotExist) && !p.validated.Load() {
-			return ErrPathNotValidated
+		switch {
+		case errors.Is(err, ErrPathNotValidated):
+			return err
+		case errors.Is(err, errPathDoesNotExist) && !p.validated.Load():
+			select {
+			case <-p.abandon:
+				return ErrPathClosed
+			default:
+				return ErrPathNotValidated
+			}
+		default:
+			return ErrPathClosed
 		}
+	}
+	return nil
+}
+
+// Close abandons a path.
+// It is not possible to close the path that’s currently active.
+// After closing, it is not possible to probe this path again.
+func (p *Path) Close() error {
+	select {
+	case <-p.abandon:
+		return nil
+	default:
+	}
+
+	if err := p.pathManager.removePath(p.id); err != nil {
 		return err
 	}
+	close(p.abandon)
 	return nil
 }
 
@@ -88,6 +121,7 @@ type pathManagerOutgoing struct {
 	scheduleSending func()
 
 	mx             sync.Mutex
+	activePath     pathID
 	pathsToProbe   []pathID
 	paths          map[pathID]*pathOutgoing
 	nextPathID     pathID
@@ -100,6 +134,8 @@ func newPathManagerOutgoing(
 	scheduleSending func(),
 ) *pathManagerOutgoing {
 	return &pathManagerOutgoing{
+		activePath:      0, // at initialization time, we're guaranteed to be using the handshake path
+		nextPathID:      1,
 		getConnID:       getConnID,
 		retireConnID:    retireConnID,
 		scheduleSending: scheduleSending,
@@ -134,6 +170,32 @@ func (pm *pathManagerOutgoing) enqueueProbe(p *Path) {
 	pm.scheduleSending()
 }
 
+func (pm *pathManagerOutgoing) removePath(id pathID) error {
+	if err := pm.removePathImpl(id); err != nil {
+		return err
+	}
+	pm.scheduleSending()
+	return nil
+}
+
+func (pm *pathManagerOutgoing) removePathImpl(id pathID) error {
+	pm.mx.Lock()
+	defer pm.mx.Unlock()
+
+	if id == pm.activePath {
+		return errors.New("cannot close active path")
+	}
+	p, ok := pm.paths[id]
+	if !ok {
+		return nil
+	}
+	if len(p.pathChallenges) > 0 {
+		pm.retireConnID(id)
+	}
+	delete(pm.paths, id)
+	return nil
+}
+
 func (pm *pathManagerOutgoing) switchToPath(id pathID) error {
 	pm.mx.Lock()
 	defer pm.mx.Unlock()
@@ -146,6 +208,7 @@ func (pm *pathManagerOutgoing) switchToPath(id pathID) error {
 		return ErrPathNotValidated
 	}
 	pm.pathToSwitchTo = p
+	pm.activePath = id
 	return nil
 }
 
@@ -161,6 +224,7 @@ func (pm *pathManagerOutgoing) NewPath(t *Transport, initialRTT time.Duration, e
 		tr:          t,
 		enablePath:  enablePath,
 		initialRTT:  initialRTT,
+		abandon:     make(chan struct{}),
 	}
 }
 
@@ -169,21 +233,19 @@ func (pm *pathManagerOutgoing) NextPathToProbe() (_ protocol.ConnectionID, _ ack
 	defer pm.mx.Unlock()
 
 	var p *pathOutgoing
-	var id pathID
-	for {
-		if len(pm.pathsToProbe) == 0 {
-			return protocol.ConnectionID{}, ackhandler.Frame{}, nil, false
-		}
-
-		id = pm.pathsToProbe[0]
-		pm.pathsToProbe = pm.pathsToProbe[1:]
-
+	id := invalidPathID
+	for _, pID := range pm.pathsToProbe {
 		var ok bool
-		// if the path doesn't exist in the map, it might have been abandoned
-		p, ok = pm.paths[id]
+		p, ok = pm.paths[pID]
 		if ok {
+			id = pID
 			break
 		}
+		// if the path doesn't exist in the map, it might have been abandoned
+		pm.pathsToProbe = pm.pathsToProbe[1:]
+	}
+	if id == invalidPathID {
+		return protocol.ConnectionID{}, ackhandler.Frame{}, nil, false
 	}
 
 	connID, ok := pm.getConnID(id)
@@ -195,6 +257,7 @@ func (pm *pathManagerOutgoing) NextPathToProbe() (_ protocol.ConnectionID, _ ack
 	_, _ = rand.Read(b[:])
 	p.pathChallenges = append(p.pathChallenges, b)
 
+	pm.pathsToProbe = pm.pathsToProbe[1:]
 	p.enablePath()
 	select {
 	case p.probeSent <- struct{}{}:



Test patch:
diff --git a/path_manager_outgoing_test.go b/path_manager_outgoing_test.go
index 54b86742..3e965b06 100644
--- a/path_manager_outgoing_test.go
+++ b/path_manager_outgoing_test.go
@@ -14,12 +14,14 @@ import (
 func TestPathManagerOutgoingPathProbing(t *testing.T) {
 	connIDs := []protocol.ConnectionID{
 		protocol.ParseConnectionID([]byte{1, 2, 3, 4, 5, 6, 7, 8}),
-		protocol.ParseConnectionID([]byte{2, 3, 4, 5, 6, 7, 8, 9}),
 	}
-	var retiredConnIDs []protocol.ConnectionID
 	pm := newPathManagerOutgoing(
-		func(id pathID) (protocol.ConnectionID, bool) { return connIDs[id], true },
-		func(id pathID) { retiredConnIDs = append(retiredConnIDs, connIDs[id]) },
+		func(id pathID) (protocol.ConnectionID, bool) {
+			connID := connIDs[0]
+			connIDs = connIDs[1:]
+			return connID, true
+		},
+		func(id pathID) { t.Fatal("didn't expect any connection ID to be retired") },
 		func() {},
 	)
 
@@ -41,7 +43,7 @@ func TestPathManagerOutgoingPathProbing(t *testing.T) {
 	connID, f, tr, ok := pm.NextPathToProbe()
 	require.True(t, ok)
 	require.Equal(t, tr1, tr)
-	require.Equal(t, connIDs[0], connID)
+	require.Equal(t, protocol.ParseConnectionID([]byte{1, 2, 3, 4, 5, 6, 7, 8}), connID)
 	require.IsType(t, &wire.PathChallengeFrame{}, f.Frame)
 	pc := f.Frame.(*wire.PathChallengeFrame)
 	require.True(t, enabled)
@@ -93,6 +95,8 @@ func TestPathManagerOutgoingPathProbing(t *testing.T) {
 	_, ok = pm.ShouldSwitchPath()
 	require.False(t, ok)
 	require.NoError(t, p.Switch())
+	// the active path can't be closed
+	require.EqualError(t, p.Close(), "cannot close active path")
 	switchToTransport, ok := pm.ShouldSwitchPath()
 	require.True(t, ok)
 	require.Equal(t, tr1, switchToTransport)
@@ -199,3 +203,59 @@ func TestPathManagerOutgoingRetransmissions(t *testing.T) {
 		t.Fatal("timeout")
 	}
 }
+
+func TestPathManagerOutgoingAbandonPath(t *testing.T) {
+	connIDs := []protocol.ConnectionID{
+		protocol.ParseConnectionID([]byte{1, 2, 3, 4, 5, 6, 7, 8}),
+	}
+	var retiredPaths []pathID
+	pm := newPathManagerOutgoing(
+		func(id pathID) (protocol.ConnectionID, bool) {
+			connID := connIDs[0]
+			connIDs = connIDs[1:]
+			return connID, true
+		},
+		func(id pathID) { retiredPaths = append(retiredPaths, id) },
+		func() {},
+	)
+
+	// path abandoned before the PATH_CHALLENGE is sent out
+	p1 := pm.NewPath(&Transport{}, time.Second, func() {})
+	errChan := make(chan error, 1)
+	go func() { errChan <- p1.Probe(context.Background()) }()
+
+	// wait for the path to be queued for probing
+	time.Sleep(scaleDuration(5 * time.Millisecond))
+
+	require.NoError(t, p1.Close())
+	// closing the path multiple times is ok
+	require.NoError(t, p1.Close())
+	require.NoError(t, p1.Close())
+	_, _, _, ok := pm.NextPathToProbe()
+	require.False(t, ok)
+
+	select {
+	case err := <-errChan:
+		require.ErrorIs(t, err, ErrPathClosed)
+	case <-time.After(time.Second):
+		t.Fatal("timeout")
+	}
+	require.Empty(t, retiredPaths)
+
+	p2 := pm.NewPath(&Transport{}, time.Second, func() {})
+	go func() { errChan <- p2.Probe(context.Background()) }()
+
+	// wait for the path to be queued for probing
+	time.Sleep(scaleDuration(5 * time.Millisecond))
+	connID, f, _, ok := pm.NextPathToProbe()
+	require.True(t, ok)
+	require.Equal(t, protocol.ParseConnectionID([]byte{1, 2, 3, 4, 5, 6, 7, 8}), connID)
+
+	require.NoError(t, p2.Close())
+	require.Equal(t, []pathID{p2.id}, retiredPaths)
+	pm.HandlePathResponseFrame(&wire.PathResponseFrame{Data: f.Frame.(*wire.PathChallengeFrame).Data})
+	_, _, _, ok = pm.NextPathToProbe()
+	require.False(t, ok)
+	// it's not possible to switch to an abandoned path
+	require.ErrorIs(t, p2.Switch(), ErrPathClosed)
+}