diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-08-11 13:22:22 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-09-23 10:02:55 +0300 |
commit | 6a5cb67aae77eb8167161bb3b027defdc96caf8f (patch) | |
tree | 9fcb58e40260b33b0ddd670f6964ccb0c08fb85f | |
parent | 3e17670ce54496373d152f4fd054f48a4f5dbd66 (diff) |
sidechannel: Check `Close()` errors returned by the waiter
We don't properly check the error when closing the `SidechannelWaiter`
in many places. Fix this and drop the corresponding linting exceptions.
-rw-r--r-- | .golangci.yml | 3 | ||||
-rw-r--r-- | client/dial_test.go | 2 | ||||
-rw-r--r-- | client/upload_pack.go | 14 | ||||
-rw-r--r-- | cmd/gitaly-hooks/hooks.go | 15 | ||||
-rw-r--r-- | internal/gitaly/hook/sidechannel_test.go | 3 | ||||
-rw-r--r-- | internal/gitaly/service/hook/pack_objects_test.go | 10 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/upload_pack_test.go | 8 | ||||
-rw-r--r-- | internal/praefect/server_factory_test.go | 2 | ||||
-rw-r--r-- | internal/sidechannel/proxy.go | 7 | ||||
-rw-r--r-- | internal/sidechannel/proxy_test.go | 2 | ||||
-rw-r--r-- | internal/sidechannel/registry_test.go | 5 | ||||
-rw-r--r-- | internal/sidechannel/sidechannel_test.go | 5 |
12 files changed, 53 insertions, 23 deletions
diff --git a/.golangci.yml b/.golangci.yml index cb43b9861..cbb624eb8 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -56,10 +56,7 @@ linters-settings: - (*database/sql.DB).Close - (*database/sql.Rows).Close - (*gitlab.com/gitlab-org/gitaly/v15/client.Pool).Close - - (*gitlab.com/gitlab-org/gitaly/v15/client.SidechannelWaiter).Close - - (*gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/hook.SidechannelWaiter).Close - (*gitlab.com/gitlab-org/gitaly/v15/internal/sidechannel.ServerConn).Close - - (*gitlab.com/gitlab-org/gitaly/v15/internal/sidechannel.Waiter).Close - (*gitlab.com/gitlab-org/gitaly/v15/internal/streamcache.pipe).Close - (*gitlab.com/gitlab-org/gitaly/v15/internal/streamcache.pipeReader).Close - (*google.golang.org/grpc.ClientConn).Close diff --git a/client/dial_test.go b/client/dial_test.go index 758ff1013..b862b4c3e 100644 --- a/client/dial_test.go +++ b/client/dial_test.go @@ -247,7 +247,7 @@ func TestDialSidechannel(t *testing.T) { return nil }) - defer scw.Close() + defer testhelper.MustClose(t, scw) req := &healthpb.HealthCheckRequest{Service: "test sidechannel"} _, err = healthpb.NewHealthClient(conn).Check(ctx, req) diff --git a/client/upload_pack.go b/client/upload_pack.go index 863465068..aeda65f6b 100644 --- a/client/upload_pack.go +++ b/client/upload_pack.go @@ -43,14 +43,24 @@ func UploadPack(ctx context.Context, conn *grpc.ClientConn, stdin io.Reader, std // UploadPackWithSidechannel proxies an SSH git-upload-pack (git fetch) // session to Gitaly using a sidechannel for the raw data transfer. -func UploadPackWithSidechannel(ctx context.Context, conn *grpc.ClientConn, reg *SidechannelRegistry, stdin io.Reader, stdout, stderr io.Writer, req *gitalypb.SSHUploadPackWithSidechannelRequest) (int32, error) { +func UploadPackWithSidechannel( + ctx context.Context, + conn *grpc.ClientConn, + reg *SidechannelRegistry, + stdin io.Reader, + stdout, stderr io.Writer, + req *gitalypb.SSHUploadPackWithSidechannelRequest, +) (int32, error) { ctx, cancel := context.WithCancel(ctx) defer cancel() ctx, wt := reg.Register(ctx, func(c SidechannelConn) error { return stream.ProxyPktLine(c, stdin, stdout, stderr) }) - defer wt.Close() + defer func() { + // We aleady check the error further down. + _ = wt.Close() + }() sshClient := gitalypb.NewSSHServiceClient(conn) if _, err := sshClient.SSHUploadPackWithSidechannel(ctx, req); err != nil { diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go index 39d0e4ba4..11601d29a 100644 --- a/cmd/gitaly-hooks/hooks.go +++ b/cmd/gitaly-hooks/hooks.go @@ -334,7 +334,10 @@ func handlePackObjectsWithSidechannel(ctx context.Context, payload git.HooksPayl if err != nil { return fmt.Errorf("SetupSidechannel: %w", err) } - defer wt.Close() + defer func() { + // We aleady check the error further down. + _ = wt.Close() + }() var glID, glUsername, gitProtocol string @@ -376,5 +379,13 @@ func handlePackObjectsWithSidechannel(ctx context.Context, payload git.HooksPayl return fmt.Errorf("call PackObjectsHookWithSidechannel: %w", err) } - return wt.Wait() + if err := wt.Wait(); err != nil { + return err + } + + if err := wt.Close(); err != nil { + return fmt.Errorf("closing sidechannel: %w", err) + } + + return nil } diff --git a/internal/gitaly/hook/sidechannel_test.go b/internal/gitaly/hook/sidechannel_test.go index 243e0de09..0cba065c9 100644 --- a/internal/gitaly/hook/sidechannel_test.go +++ b/internal/gitaly/hook/sidechannel_test.go @@ -47,7 +47,7 @@ func testSidechannelWithRuntimeDir(t *testing.T, runtimeDir string) { }, ) require.NoError(t, err) - defer wt.Close() + defer testhelper.MustClose(t, wt) require.DirExists(t, wt.socketDir) @@ -85,7 +85,6 @@ func testSidechannelCleanupWithRuntimeDir(t *testing.T, runtimeDir string) { func(c *net.UnixConn) error { return nil }, ) require.NoError(t, err) - defer wt.Close() require.DirExists(t, wt.socketDir) _ = wt.Close() diff --git a/internal/gitaly/service/hook/pack_objects_test.go b/internal/gitaly/service/hook/pack_objects_test.go index 0bf4262da..403f3fe77 100644 --- a/internal/gitaly/service/hook/pack_objects_test.go +++ b/internal/gitaly/service/hook/pack_objects_test.go @@ -134,7 +134,7 @@ func testServerPackObjectsHookSeparateContextWithRuntimeDir(t *testing.T, ctx co }, ) require.NoError(t, err) - defer wt1.Close() + defer testhelper.MustClose(t, wt1) go func() { defer wg.Done() @@ -174,7 +174,7 @@ func testServerPackObjectsHookSeparateContextWithRuntimeDir(t *testing.T, ctx co }, ) require.NoError(t, err) - defer wt2.Close() + defer testhelper.MustClose(t, wt2) go func() { defer wg.Done() @@ -240,7 +240,7 @@ func testServerPackObjectsHookUsesCache(t *testing.T, ctx context.Context, runti }, ) require.NoError(t, err) - defer wt.Close() + defer testhelper.MustClose(t, wt) client, conn := newHooksClient(t, cfg.SocketPath) defer conn.Close() @@ -346,7 +346,7 @@ func testServerPackObjectsHookWithSidechannelWithRuntimeDir(t *testing.T, ctx co }, ) require.NoError(t, err) - defer wt.Close() + defer testhelper.MustClose(t, wt) client, conn := newHooksClient(t, cfg.SocketPath) defer conn.Close() @@ -539,7 +539,7 @@ func testServerPackObjectsHookWithSidechannelCanceledWithRuntimeDir(t *testing.T }, ) require.NoError(t, err) - defer wt.Close() + defer testhelper.MustClose(t, wt) cfg.SocketPath = runHooksServer(t, cfg, nil) repo, _ := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{ diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go index 02ca6296e..1da5527da 100644 --- a/internal/gitaly/service/smarthttp/upload_pack_test.go +++ b/internal/gitaly/service/smarthttp/upload_pack_test.go @@ -6,6 +6,7 @@ import ( "bytes" "context" "encoding/binary" + "errors" "fmt" "io" "path/filepath" @@ -613,7 +614,6 @@ func makePostUploadPackWithSidechannelRequest(ctx context.Context, t *testing.T, return <-errC }) - defer waiter.Close() rpcRequest := &gitalypb.PostUploadPackWithSidechannelRequest{ Repository: in.GetRepository(), @@ -622,7 +622,11 @@ func makePostUploadPackWithSidechannelRequest(ctx context.Context, t *testing.T, } _, err := client.PostUploadPackWithSidechannel(ctxOut, rpcRequest) if err == nil { - require.NoError(t, waiter.Close()) + testhelper.MustClose(t, waiter) + } else if err := waiter.Close(); err != nil && !errors.Is(err, sidechannel.ErrCallbackDidNotRun) { + // When the request failed the sidechannel may not even have been used, so we need + // to catch the `ErrCallbackDidNotRun` error here. + require.NoError(t, err) } return responseBuffer, err diff --git a/internal/praefect/server_factory_test.go b/internal/praefect/server_factory_test.go index 7fd850e21..598197311 100644 --- a/internal/praefect/server_factory_test.go +++ b/internal/praefect/server_factory_test.go @@ -165,7 +165,7 @@ func TestServerFactory(t *testing.T) { return nil }) - defer waiter.Close() + defer testhelper.MustClose(t, waiter) _, err = gitalypb.NewSmartHTTPServiceClient(cc).PostUploadPackWithSidechannel(ctx, &gitalypb.PostUploadPackWithSidechannelRequest{Repository: repo}, diff --git a/internal/sidechannel/proxy.go b/internal/sidechannel/proxy.go index 6da61274f..ee92baebf 100644 --- a/internal/sidechannel/proxy.go +++ b/internal/sidechannel/proxy.go @@ -18,14 +18,19 @@ func NewUnaryProxy(registry *Registry) grpc.UnaryClientInterceptor { } ctx, waiter := RegisterSidechannel(ctx, registry, proxy(ctx)) - defer waiter.Close() + defer func() { + // We aleady check the error further down. + _ = waiter.Close() + }() if err := invoker(ctx, method, req, reply, cc, opts...); err != nil { return err } + if err := waiter.Close(); err != nil && err != ErrCallbackDidNotRun { return fmt.Errorf("sidechannel: proxy callback: %w", err) } + return nil } } diff --git a/internal/sidechannel/proxy_test.go b/internal/sidechannel/proxy_test.go index ed2f3e2a8..e61f8a9b1 100644 --- a/internal/sidechannel/proxy_test.go +++ b/internal/sidechannel/proxy_test.go @@ -169,7 +169,7 @@ func testStreamProxy(t *testing.T, closeWrite bool) { conn, registry := dial(t, proxyAddr) ctx, waiter := RegisterSidechannel(ctx, registry, testProxyClient(closeWrite)) - defer waiter.Close() + defer testhelper.MustClose(t, waiter) client, err := gitalypb.NewSSHServiceClient(conn).SSHUploadPack(ctx) require.NoError(t, err) diff --git a/internal/sidechannel/registry_test.go b/internal/sidechannel/registry_test.go index 8a286b617..d1a37b927 100644 --- a/internal/sidechannel/registry_test.go +++ b/internal/sidechannel/registry_test.go @@ -13,6 +13,7 @@ import ( "testing" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" ) func TestRegistry(t *testing.T) { @@ -25,7 +26,7 @@ func TestRegistry(t *testing.T) { <-triggerCallback return nil }) - defer waiter.Close() + defer testhelper.MustClose(t, waiter) require.Equal(t, 1, registry.waiting()) @@ -57,7 +58,7 @@ func TestRegistry(t *testing.T) { return conn.CloseWrite() }) - defer waiter.Close() + defer testhelper.MustClose(t, waiter) require.NoError(t, registry.receive(waiter.id, client)) require.NoError(t, waiter.Close()) diff --git a/internal/sidechannel/sidechannel_test.go b/internal/sidechannel/sidechannel_test.go index 7043e1642..bcc24c7a8 100644 --- a/internal/sidechannel/sidechannel_test.go +++ b/internal/sidechannel/sidechannel_test.go @@ -187,7 +187,10 @@ func call(ctx context.Context, conn *grpc.ClientConn, registry *Registry, handle client := healthpb.NewHealthClient(conn) ctxOut, waiter := RegisterSidechannel(ctx, registry, handler) - defer waiter.Close() + defer func() { + // We aleady check the error further down. + _ = waiter.Close() + }() if _, err := client.Check(ctxOut, &healthpb.HealthCheckRequest{}); err != nil { return err |