diff --git a/builtin/provisioners/file/resource_provisioner.go b/builtin/provisioners/file/resource_provisioner.go index 559be022d..2625d6643 100644 --- a/builtin/provisioners/file/resource_provisioner.go +++ b/builtin/provisioners/file/resource_provisioner.go @@ -6,7 +6,6 @@ import ( "fmt" "io/ioutil" "os" - "sync" "github.com/hashicorp/terraform/communicator" "github.com/hashicorp/terraform/configs/configschema" @@ -16,13 +15,17 @@ import ( ) func New() provisioners.Interface { - return &provisioner{} + ctx, cancel := context.WithCancel(context.Background()) + return &provisioner{ + ctx: ctx, + cancel: cancel, + } } type provisioner struct { - // this stored from the running context, so that Stop() can - // cancel the transfer - mu sync.Mutex + // We store a context here tied to the lifetime of the provisioner. + // This allows the Stop method to cancel any in-flight requests. + ctx context.Context cancel context.CancelFunc } @@ -71,11 +74,6 @@ func (p *provisioner) ValidateProvisionerConfig(req provisioners.ValidateProvisi } func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) { - p.mu.Lock() - ctx, cancel := context.WithCancel(context.Background()) - p.cancel = cancel - p.mu.Unlock() - comm, err := communicator.New(req.Connection) if err != nil { resp.Diagnostics = resp.Diagnostics.Append(err) @@ -94,7 +92,7 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques // Begin the file copy dst := req.Config.GetAttr("destination").AsString() - if err := copyFiles(ctx, comm, src, dst); err != nil { + if err := copyFiles(p.ctx, comm, src, dst); err != nil { resp.Diagnostics = resp.Diagnostics.Append(err) return resp } @@ -178,12 +176,11 @@ func copyFiles(ctx context.Context, comm communicator.Communicator, src, dst str } func (p *provisioner) Stop() error { - p.mu.Lock() - defer p.mu.Unlock() p.cancel() return nil } func (p *provisioner) Close() error { + p.cancel() return nil } diff --git a/builtin/provisioners/file/resource_provisioner_test.go b/builtin/provisioners/file/resource_provisioner_test.go index 9532fd517..080c76a74 100644 --- a/builtin/provisioners/file/resource_provisioner_test.go +++ b/builtin/provisioners/file/resource_provisioner_test.go @@ -95,3 +95,10 @@ func TestResourceProvider_Validate_bad_to_many_src(t *testing.T) { t.Fatal("Should have errors") } } + +// Validate that Stop can Close can be called even when not provisioning. +func TestResourceProvisioner_StopClose(t *testing.T) { + p := New() + p.Stop() + p.Close() +} diff --git a/builtin/provisioners/local-exec/resource_provisioner.go b/builtin/provisioners/local-exec/resource_provisioner.go index 6b03725dc..fb58ad254 100644 --- a/builtin/provisioners/local-exec/resource_provisioner.go +++ b/builtin/provisioners/local-exec/resource_provisioner.go @@ -7,7 +7,6 @@ import ( "os" "os/exec" "runtime" - "sync" "github.com/armon/circbuf" "github.com/hashicorp/terraform/configs/configschema" @@ -24,13 +23,17 @@ const ( ) func New() provisioners.Interface { - return &provisioner{} + ctx, cancel := context.WithCancel(context.Background()) + return &provisioner{ + ctx: ctx, + cancel: cancel, + } } type provisioner struct { - // this stored from the running context, so that Stop() can cancel the - // command - mu sync.Mutex + // We store a context here tied to the lifetime of the provisioner. + // This allows the Stop method to cancel any in-flight requests. + ctx context.Context cancel context.CancelFunc } @@ -68,11 +71,6 @@ func (p *provisioner) ValidateProvisionerConfig(req provisioners.ValidateProvisi } func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) { - p.mu.Lock() - ctx, cancel := context.WithCancel(context.Background()) - p.cancel = cancel - p.mu.Unlock() - command := req.Config.GetAttr("command").AsString() if command == "" { resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("local-exec provisioner command must be a non-empty string")) @@ -127,7 +125,7 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques cmdEnv = append(cmdEnv, env...) // Setup the command - cmd := exec.CommandContext(ctx, cmdargs[0], cmdargs[1:]...) + cmd := exec.CommandContext(p.ctx, cmdargs[0], cmdargs[1:]...) cmd.Stderr = pw cmd.Stdout = pw // Dir specifies the working directory of the command. @@ -165,7 +163,7 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques // copyOutput goroutine will just hang out until exit. select { case <-copyDoneCh: - case <-ctx.Done(): + case <-p.ctx.Done(): } if err != nil { @@ -178,13 +176,12 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques } func (p *provisioner) Stop() error { - p.mu.Lock() - defer p.mu.Unlock() p.cancel() return nil } func (p *provisioner) Close() error { + p.cancel() return nil } diff --git a/builtin/provisioners/local-exec/resource_provisioner_test.go b/builtin/provisioners/local-exec/resource_provisioner_test.go index f828cc8fd..235e69646 100644 --- a/builtin/provisioners/local-exec/resource_provisioner_test.go +++ b/builtin/provisioners/local-exec/resource_provisioner_test.go @@ -197,3 +197,10 @@ BAR 1 true` t.Errorf("wrong output\ngot: %s\nwant: %s", got, want) } } + +// Validate that Stop can Close can be called even when not provisioning. +func TestResourceProvisioner_StopClose(t *testing.T) { + p := New() + p.Stop() + p.Close() +} diff --git a/builtin/provisioners/remote-exec/resource_provisioner.go b/builtin/provisioners/remote-exec/resource_provisioner.go index adbb0d50d..088584ce5 100644 --- a/builtin/provisioners/remote-exec/resource_provisioner.go +++ b/builtin/provisioners/remote-exec/resource_provisioner.go @@ -10,7 +10,6 @@ import ( "log" "os" "strings" - "sync" "github.com/hashicorp/terraform/communicator" "github.com/hashicorp/terraform/communicator/remote" @@ -21,13 +20,17 @@ import ( ) func New() provisioners.Interface { - return &provisioner{} + ctx, cancel := context.WithCancel(context.Background()) + return &provisioner{ + ctx: ctx, + cancel: cancel, + } } type provisioner struct { - // this stored from the running context, so that Stop() can cancel the - // command - mu sync.Mutex + // We store a context here tied to the lifetime of the provisioner. + // This allows the Stop method to cancel any in-flight requests. + ctx context.Context cancel context.CancelFunc } @@ -82,11 +85,6 @@ func (p *provisioner) ValidateProvisionerConfig(req provisioners.ValidateProvisi } func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) { - p.mu.Lock() - ctx, cancel := context.WithCancel(context.Background()) - p.cancel = cancel - p.mu.Unlock() - comm, err := communicator.New(req.Connection) if err != nil { resp.Diagnostics = resp.Diagnostics.Append(err) @@ -104,7 +102,7 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques } // Copy and execute each script - if err := runScripts(ctx, req.UIOutput, comm, scripts); err != nil { + if err := runScripts(p.ctx, req.UIOutput, comm, scripts); err != nil { resp.Diagnostics = resp.Diagnostics.Append(err) return resp } @@ -113,13 +111,12 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques } func (p *provisioner) Stop() error { - p.mu.Lock() - defer p.mu.Unlock() p.cancel() return nil } func (p *provisioner) Close() error { + p.cancel() return nil } diff --git a/builtin/provisioners/remote-exec/resource_provisioner_test.go b/builtin/provisioners/remote-exec/resource_provisioner_test.go index 727d1735f..16c115261 100644 --- a/builtin/provisioners/remote-exec/resource_provisioner_test.go +++ b/builtin/provisioners/remote-exec/resource_provisioner_test.go @@ -254,3 +254,10 @@ func TestProvisionerTimeout(t *testing.T) { t.Fatal(err) } } + +// Validate that Stop can Close can be called even when not provisioning. +func TestResourceProvisioner_StopClose(t *testing.T) { + p := New() + p.Stop() + p.Close() +}