Repository: scionproto/scion
Pull request #4314 (2023-02-03 14:50:38)

control: close connection in SVC resolver
When performing a service address resolution with `svc.Resolver.LookupSVC`, a new connection was opened but never closed.
One possible reason for this bug was that the unclear responsibility for closing the connection; the `svc.RoundTripper.RoundTrip` function closes the connection if the context ends, but not otherwise.

Fix this by always closing the conn in `CloseConnOnDone`.
The cancel function of `CloseConnOnDone` now also blocks until the connection is closed (relevant mostly for testing) and returns any errors from closing.
Also move the `CloseConnOnDone` to `LookupSVC`, so the responsibility for closing the connection is right next to where it's opened.

<!-- Reviewable:start -->
- - -
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/scionproto/scion/4314)
<!-- Reviewable:end -->



Patch:
diff --git a/private/svc/BUILD.bazel b/private/svc/BUILD.bazel
index 377aaf663..4d6724127 100644
--- a/private/svc/BUILD.bazel
+++ b/private/svc/BUILD.bazel
@@ -11,6 +11,7 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/addr:go_default_library",
+        "//pkg/log:go_default_library",
         "//pkg/private/common:go_default_library",
         "//pkg/private/serrors:go_default_library",
         "//pkg/proto/control_plane:go_default_library",
diff --git a/private/svc/internal/ctxconn/ctxconn.go b/private/svc/internal/ctxconn/ctxconn.go
index b1c3a142c..83f8531ee 100644
--- a/private/svc/internal/ctxconn/ctxconn.go
+++ b/private/svc/internal/ctxconn/ctxconn.go
@@ -29,41 +29,27 @@ type DeadlineCloser interface {
 	io.Closer
 }
 
-// CancelFunc can be used to shut down context watchers. It is safe to call the
-// cancel function multiple times.
-type CancelFunc func()
-
 // CloseConnOnDone closes conn whenever ctx is Done. This includes if ctx is
 // canceled via its cancellation function.
 //
-// Call the returned cancellation function to free up resources. Calling this
-// function does not guarantee that the connection has been closed. It is not
-// safe to call the returned function multiple times at the same time.
-func CloseConnOnDone(ctx context.Context, conn DeadlineCloser) CancelFunc {
+// Call the returned cancellation function to free up resources and closing the
+// connection. The cancellation function returns any error from closing the connection.
+func CloseConnOnDone(ctx context.Context, conn DeadlineCloser) func() error {
 	if deadline, ok := ctx.Deadline(); ok {
 		// ignore error; if deadline cannot be set, we'll just close the conn
 		// when the context is Done anyway.
 		_ = conn.SetDeadline(deadline)
 	}
 
-	cancelSignal := make(chan struct{})
+	ctx, cancelCtx := context.WithCancel(ctx)
+	errChan := make(chan error)
 	go func() {
 		defer log.HandlePanic()
-		select {
-		case <-ctx.Done():
-			if err := conn.Close(); err != nil {
-				log.Info("Error closing conn when ctx canceled", "err", err)
-			}
-		case <-cancelSignal:
-			// shut down goroutine, free up resources
-			return
-		}
+		<-ctx.Done()
+		errChan <- conn.Close()
 	}()
-	return func() {
-		select {
-		case <-cancelSignal:
-		default:
-			close(cancelSignal)
-		}
+	return func() error {
+		cancelCtx()
+		return <-errChan
 	}
 }
diff --git a/private/svc/resolver.go b/private/svc/resolver.go
index cb21d1f28..ad0c2190c 100644
--- a/private/svc/resolver.go
+++ b/private/svc/resolver.go
@@ -23,6 +23,7 @@ import (
 	"google.golang.org/protobuf/proto"
 
 	"github.com/scionproto/scion/pkg/addr"
+	"github.com/scionproto/scion/pkg/log"
 	"github.com/scionproto/scion/pkg/private/common"
 	"github.com/scionproto/scion/pkg/private/serrors"
 	cppb "github.com/scionproto/scion/pkg/proto/control_plane"
@@ -83,6 +84,12 @@ func (r *Resolver) LookupSVC(ctx context.Context, p snet.Path, svc addr.HostSVC)
 		ext.Error.Set(span, true)
 		return nil, serrors.Wrap(errRegistration, err)
 	}
+	cancelF := ctxconn.CloseConnOnDone(ctx, conn)
+	defer func() {
+		if err := cancelF(); err != nil {
+			log.Info("Error closing conn", "err", err)
+		}
+	}()
 
 	requestPacket := &snet.Packet{
 		PacketInfo: snet.PacketInfo{
@@ -137,9 +144,6 @@ type roundTripper struct{}
 func (roundTripper) RoundTrip(ctx context.Context, c snet.PacketConn, pkt *snet.Packet,
 	u *net.UDPAddr) (*Reply, error) {
 
-	cancelF := ctxconn.CloseConnOnDone(ctx, c)
-	defer cancelF()
-
 	if pkt == nil {
 		return nil, errNilPacket
 	}


Test patch:
diff --git a/private/svc/internal/ctxconn/ctxconn_test.go b/private/svc/internal/ctxconn/ctxconn_test.go
index 0f362a5de..bc9d31931 100644
--- a/private/svc/internal/ctxconn/ctxconn_test.go
+++ b/private/svc/internal/ctxconn/ctxconn_test.go
@@ -16,6 +16,7 @@ package ctxconn
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
@@ -25,15 +26,26 @@ import (
 	"github.com/scionproto/scion/private/svc/internal/ctxconn/mock_ctxconn"
 )
 
-const baseUnit = time.Millisecond
-
 func TestCloseConeOnDone(t *testing.T) {
 
-	t.Run("if no deadline and no ctx canceled, close it not called", func(t *testing.T) {
+	t.Run("if no deadline and no ctx canceled, close is called once", func(t *testing.T) {
 		ctrl := gomock.NewController(t)
 		defer ctrl.Finish()
-		cancelFunc := CloseConnOnDone(context.Background(), nil)
-		assert.NotPanics(t, func() { cancelFunc() })
+		closer := mock_ctxconn.NewMockDeadlineCloser(ctrl)
+		closer.EXPECT().Close()
+		cancelFunc := CloseConnOnDone(context.Background(), closer)
+		err := cancelFunc()
+		assert.NoError(t, err)
+	})
+	t.Run("close error is returned", func(t *testing.T) {
+		ctrl := gomock.NewController(t)
+		defer ctrl.Finish()
+		closer := mock_ctxconn.NewMockDeadlineCloser(ctrl)
+		testErr := errors.New("test")
+		closer.EXPECT().Close().Return(testErr)
+		cancelFunc := CloseConnOnDone(context.Background(), closer)
+		err := cancelFunc()
+		assert.Equal(t, testErr, err)
 	})
 	t.Run("if no deadline and ctx canceled, close is called once", func(t *testing.T) {
 		ctrl := gomock.NewController(t)
@@ -43,20 +55,22 @@ func TestCloseConeOnDone(t *testing.T) {
 		closer := mock_ctxconn.NewMockDeadlineCloser(ctrl)
 		closer.EXPECT().Close()
 		cancelFunc := CloseConnOnDone(ctx, closer)
-		time.Sleep(20 * baseUnit)
-		cancelFunc()
+		err := cancelFunc()
+		assert.NoError(t, err)
 	})
 
 	t.Run("if deadline expires, close is called once", func(t *testing.T) {
 		ctrl := gomock.NewController(t)
 		defer ctrl.Finish()
-		deadline := time.Now().Add(0)
-		ctx, cancelF := context.WithDeadline(context.Background(), deadline)
-		defer cancelF()
+		deadline := time.Now().Add(20 * time.Millisecond)
+		ctx, ctxCancelF := context.WithDeadline(context.Background(), deadline)
+
 		closer := mock_ctxconn.NewMockDeadlineCloser(ctrl)
 		closer.EXPECT().Close()
 		closer.EXPECT().SetDeadline(deadline)
-		CloseConnOnDone(ctx, closer)
-		time.Sleep(20 * baseUnit)
+		cancelFunc := CloseConnOnDone(ctx, closer)
+		ctxCancelF() // Pretend that we hit the deadline
+		err := cancelFunc()
+		assert.NoError(t, err)
 	})
 }
diff --git a/private/svc/resolver_test.go b/private/svc/resolver_test.go
index 926d86826..72325863e 100644
--- a/private/svc/resolver_test.go
+++ b/private/svc/resolver_test.go
@@ -68,6 +68,7 @@ func TestResolver(t *testing.T) {
 		mockPacketDispatcherService.EXPECT().Register(gomock.Any(), srcIA,
 			&net.UDPAddr{IP: net.IP{192, 0, 2, 1}},
 			addr.SvcNone).Return(mockConn, uint16(42), nil)
+		mockConn.EXPECT().Close()
 		mockRoundTripper := mock_svc.NewMockRoundTripper(ctrl)
 		mockRoundTripper.EXPECT().RoundTrip(gomock.Any(), gomock.Any(), gomock.Any(),
 			gomock.Any()).Do(