From 20c05f8e9902bf825de84583f8da05799659cb92 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Tue, 3 May 2022 07:36:31 -0500 Subject: [PATCH 1/2] fix: additional provisionerd test double closes https://github.com/coder/coder/runs/6266376800?check_suite_focus=true --- provisionerd/provisionerd_test.go | 85 +++++++++++++++++-------------- 1 file changed, 48 insertions(+), 37 deletions(-) diff --git a/provisionerd/provisionerd_test.go b/provisionerd/provisionerd_test.go index 1999061dd49e4..ddfe2f0f1da28 100644 --- a/provisionerd/provisionerd_test.go +++ b/provisionerd/provisionerd_test.go @@ -130,8 +130,11 @@ func TestProvisionerd(t *testing.T) { // Ensures tars with "../../../etc/passwd" as the path // are not allowed to run, and will fail the job. t.Parallel() - var complete sync.Once - completeChan := make(chan struct{}) + var ( + completeChan = make(chan struct{}) + completeOnce sync.Once + ) + closer := createProvisionerd(t, func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) { return createProvisionerDaemonClient(t, provisionerDaemonTestServer{ acquireJob: func(ctx context.Context, _ *proto.Empty) (*proto.AcquiredJob, error) { @@ -150,9 +153,7 @@ func TestProvisionerd(t *testing.T) { }, updateJob: noopUpdateJob, failJob: func(ctx context.Context, job *proto.FailedJob) (*proto.Empty, error) { - complete.Do(func() { - close(completeChan) - }) + completeOnce.Do(func() { close(completeChan) }) return &proto.Empty{}, nil }, }), nil @@ -165,8 +166,11 @@ func TestProvisionerd(t *testing.T) { t.Run("RunningPeriodicUpdate", func(t *testing.T) { t.Parallel() - var complete sync.Once - completeChan := make(chan struct{}) + var ( + completeChan = make(chan struct{}) + completeOnce sync.Once + ) + closer := createProvisionerd(t, func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) { return createProvisionerDaemonClient(t, provisionerDaemonTestServer{ acquireJob: func(ctx context.Context, _ *proto.Empty) (*proto.AcquiredJob, error) { @@ -184,9 +188,7 @@ func TestProvisionerd(t *testing.T) { }, nil }, updateJob: func(ctx context.Context, update *proto.UpdateJobRequest) (*proto.UpdateJobResponse, error) { - complete.Do(func() { - close(completeChan) - }) + completeOnce.Do(func() { close(completeChan) }) return &proto.UpdateJobResponse{}, nil }, failJob: func(ctx context.Context, job *proto.FailedJob) (*proto.Empty, error) { @@ -212,14 +214,15 @@ func TestProvisionerd(t *testing.T) { didLog atomic.Bool didAcquireJob atomic.Bool didDryRun atomic.Bool + completeChan = make(chan struct{}) + completeOnce sync.Once ) - var complete sync.Once - completeChan := make(chan struct{}) + closer := createProvisionerd(t, func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) { return createProvisionerDaemonClient(t, provisionerDaemonTestServer{ acquireJob: func(ctx context.Context, _ *proto.Empty) (*proto.AcquiredJob, error) { if didAcquireJob.Load() { - complete.Do(func() { + completeOnce.Do(func() { close(completeChan) }) return &proto.AcquiredJob{}, nil @@ -315,16 +318,15 @@ func TestProvisionerd(t *testing.T) { didComplete atomic.Bool didLog atomic.Bool didAcquireJob atomic.Bool + completeChan = make(chan struct{}) + completeOnce sync.Once ) - var complete sync.Once - completeChan := make(chan struct{}) + closer := createProvisionerd(t, func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) { return createProvisionerDaemonClient(t, provisionerDaemonTestServer{ acquireJob: func(ctx context.Context, _ *proto.Empty) (*proto.AcquiredJob, error) { if didAcquireJob.Load() { - complete.Do(func() { - close(completeChan) - }) + completeOnce.Do(func() { close(completeChan) }) return &proto.AcquiredJob{}, nil } didAcquireJob.Store(true) @@ -386,13 +388,15 @@ func TestProvisionerd(t *testing.T) { var ( didFail atomic.Bool didAcquireJob atomic.Bool + completeChan = make(chan struct{}) + completeOnce sync.Once ) - completeChan := make(chan struct{}) + closer := createProvisionerd(t, func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) { return createProvisionerDaemonClient(t, provisionerDaemonTestServer{ acquireJob: func(ctx context.Context, _ *proto.Empty) (*proto.AcquiredJob, error) { if didAcquireJob.Load() { - close(completeChan) + completeOnce.Do(func() { close(completeChan) }) return &proto.AcquiredJob{}, nil } didAcquireJob.Store(true) @@ -585,10 +589,15 @@ func TestProvisionerd(t *testing.T) { t.Run("ReconnectAndFail", func(t *testing.T) { t.Parallel() - var second atomic.Bool - failChan := make(chan struct{}) - failedChan := make(chan struct{}) - completeChan := make(chan struct{}) + var ( + second atomic.Bool + failChan = make(chan struct{}) + failOnce sync.Once + failedChan = make(chan struct{}) + failedOnce sync.Once + completeChan = make(chan struct{}) + completeOnce sync.Once + ) server := createProvisionerd(t, func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) { client := createProvisionerDaemonClient(t, provisionerDaemonTestServer{ acquireJob: func(ctx context.Context, _ *proto.Empty) (*proto.AcquiredJob, error) { @@ -613,10 +622,10 @@ func TestProvisionerd(t *testing.T) { }, failJob: func(ctx context.Context, job *proto.FailedJob) (*proto.Empty, error) { if second.Load() { - close(completeChan) + completeOnce.Do(func() { close(completeChan) }) return &proto.Empty{}, nil } - close(failChan) + failOnce.Do(func() { close(failChan) }) <-failedChan return &proto.Empty{}, nil }, @@ -626,7 +635,7 @@ func TestProvisionerd(t *testing.T) { <-failChan _ = client.DRPCConn().Close() second.Store(true) - close(failedChan) + failedOnce.Do(func() { close(failedChan) }) }() } return client, nil @@ -651,18 +660,20 @@ func TestProvisionerd(t *testing.T) { t.Run("ReconnectAndComplete", func(t *testing.T) { t.Parallel() - var completed sync.Once - var second atomic.Bool - failChan := make(chan struct{}) - failedChan := make(chan struct{}) - completeChan := make(chan struct{}) + var ( + second atomic.Bool + failChan = make(chan struct{}) + failOnce sync.Once + failedChan = make(chan struct{}) + failedOnce sync.Once + completeChan = make(chan struct{}) + completeOnce sync.Once + ) server := createProvisionerd(t, func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) { client := createProvisionerDaemonClient(t, provisionerDaemonTestServer{ acquireJob: func(ctx context.Context, _ *proto.Empty) (*proto.AcquiredJob, error) { if second.Load() { - completed.Do(func() { - close(completeChan) - }) + completeOnce.Do(func() { close(completeChan) }) return &proto.AcquiredJob{}, nil } return &proto.AcquiredJob{ @@ -688,7 +699,7 @@ func TestProvisionerd(t *testing.T) { if second.Load() { return &proto.Empty{}, nil } - close(failChan) + failOnce.Do(func() { close(failChan) }) <-failedChan return &proto.Empty{}, nil }, @@ -698,7 +709,7 @@ func TestProvisionerd(t *testing.T) { <-failChan _ = client.DRPCConn().Close() second.Store(true) - close(failedChan) + failedOnce.Do(func() { close(failedChan) }) }() } return client, nil From 356db63da3cda13669246a724cc4f20f25472889 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Tue, 3 May 2022 07:42:18 -0500 Subject: [PATCH 2/2] use CAS --- provisionerd/provisionerd_test.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/provisionerd/provisionerd_test.go b/provisionerd/provisionerd_test.go index ddfe2f0f1da28..c4b82472e0ef5 100644 --- a/provisionerd/provisionerd_test.go +++ b/provisionerd/provisionerd_test.go @@ -221,13 +221,11 @@ func TestProvisionerd(t *testing.T) { closer := createProvisionerd(t, func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) { return createProvisionerDaemonClient(t, provisionerDaemonTestServer{ acquireJob: func(ctx context.Context, _ *proto.Empty) (*proto.AcquiredJob, error) { - if didAcquireJob.Load() { - completeOnce.Do(func() { - close(completeChan) - }) + if !didAcquireJob.CAS(false, true) { + completeOnce.Do(func() { close(completeChan) }) return &proto.AcquiredJob{}, nil } - didAcquireJob.Store(true) + return &proto.AcquiredJob{ JobId: "test", Provisioner: "someprovisioner", @@ -325,11 +323,11 @@ func TestProvisionerd(t *testing.T) { closer := createProvisionerd(t, func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) { return createProvisionerDaemonClient(t, provisionerDaemonTestServer{ acquireJob: func(ctx context.Context, _ *proto.Empty) (*proto.AcquiredJob, error) { - if didAcquireJob.Load() { + if !didAcquireJob.CAS(false, true) { completeOnce.Do(func() { close(completeChan) }) return &proto.AcquiredJob{}, nil } - didAcquireJob.Store(true) + return &proto.AcquiredJob{ JobId: "test", Provisioner: "someprovisioner", @@ -395,11 +393,11 @@ func TestProvisionerd(t *testing.T) { closer := createProvisionerd(t, func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) { return createProvisionerDaemonClient(t, provisionerDaemonTestServer{ acquireJob: func(ctx context.Context, _ *proto.Empty) (*proto.AcquiredJob, error) { - if didAcquireJob.Load() { + if !didAcquireJob.CAS(false, true) { completeOnce.Do(func() { close(completeChan) }) return &proto.AcquiredJob{}, nil } - didAcquireJob.Store(true) + return &proto.AcquiredJob{ JobId: "test", Provisioner: "someprovisioner",