Skip to content

Commit

Permalink
fix: ensure proper closure of client.Client.conn with finalizer
Browse files Browse the repository at this point in the history
Always close the connection, even if we forgot to do this ourselves.

Signed-off-by: Dmitriy Matrenichev <[email protected]>
  • Loading branch information
DmitriyMV committed Jan 31, 2025
1 parent 19040ff commit 673ca4b
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 0 deletions.
8 changes: 8 additions & 0 deletions pkg/machinery/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"errors"
"fmt"
"io"
"runtime"
"time"

cosiv1alpha1 "github.com/cosi-project/runtime/api/v1alpha1"
Expand Down Expand Up @@ -173,11 +174,16 @@ func New(_ context.Context, opts ...OptionFunc) (c *Client, err error) {
c.Inspect = &InspectClient{c.InspectClient}
c.COSI = state.WrapCore(client.NewAdapter(cosiv1alpha1.NewStateClient(c.conn)))

// Just like Go team does in their os.File internals, lets ensure that we always close the connection.
runtime.SetFinalizer(c.conn, clientFinalizer)

return c, nil
}

// Close shuts down client protocol.
func (c *Client) Close() error {
runtime.SetFinalizer(c.conn, nil) // remove so it doesn't get closed twice

return c.conn.Close()
}

Expand Down Expand Up @@ -1011,3 +1017,5 @@ func (c *Client) BlockDeviceWipe(ctx context.Context, req *storageapi.BlockDevic

return err
}

var clientFinalizer = io.Closer.Close
61 changes: 61 additions & 0 deletions pkg/machinery/client/client_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/.

package client_test

import (
"context"
"io"
"runtime"
"testing"
"time"

"github.com/siderolabs/gen/xtesting/must"
"github.com/stretchr/testify/require"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"

"github.com/siderolabs/talos/pkg/machinery/client"
)

func TestNew(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()

fCalled := make(chan struct{})

prev := client.SetClientFinalizer(func(closer io.Closer) error {
defer close(fCalled)

require.NoError(t, closer.Close())

return nil
})
defer client.SetClientFinalizer(prev)

c := must.Value(
client.New(
ctx,
client.WithUnixSocket("/path/to/socket"),
client.WithGRPCDialOptions(
grpc.WithTransportCredentials(insecure.NewCredentials()),
),
),
)(t)

runtime.SetFinalizer(c, func(c *client.Client) { t.Log("client finalizer set and called") })

for {
select {
case <-fCalled:
t.Log("client conn finalized")

return
case <-ctx.Done():
require.Fail(t, "client finalizer not called")
default:
runtime.GC()
}
}
}
7 changes: 7 additions & 0 deletions pkg/machinery/client/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package client

import (
"crypto/tls"
"io"

clientconfig "github.com/siderolabs/talos/pkg/machinery/client/config"
)
Expand All @@ -17,3 +18,9 @@ func ReduceURLsToAddresses(endpoints []string) []string {
func BuildTLSConfig(configContext *clientconfig.Context) (*tls.Config, error) {
return buildTLSConfig(configContext)
}

func SetClientFinalizer(fn func(io.Closer) error) (old func(io.Closer) error) {
old, clientFinalizer = clientFinalizer, fn

return old
}

0 comments on commit 673ca4b

Please sign in to comment.