From d869923103a308655d7d554ed6453b638d2d4f5a Mon Sep 17 00:00:00 2001 From: 1garo <44412643+1garo@users.noreply.github.com> Date: Thu, 25 Apr 2024 11:25:13 -0300 Subject: [PATCH] Review and order locked struct fields (#1493) Signed-off-by: 1garo Signed-off-by: Christian Mesh Co-authored-by: Christian Mesh --- .../backend/remote-state/consul/client.go | 8 +++---- internal/checks/state.go | 3 +-- internal/cloud/state.go | 3 +-- internal/cloud/tfe_client_mock.go | 1 - internal/command/clistate/local_state.go | 3 +-- internal/command/clistate/state.go | 2 +- internal/command/views/hook_count.go | 2 +- internal/command/views/hook_json.go | 4 ++-- internal/command/views/hook_ui.go | 4 ++-- internal/dag/walk.go | 11 +++++----- internal/getproviders/memoize_source.go | 2 +- internal/lang/scope.go | 2 +- .../legacy/helper/schema/field_writer_map.go | 3 +-- .../legacy/helper/schema/resource_diff.go | 5 ++--- internal/legacy/tofu/state.go | 9 +++----- internal/logging/panic.go | 1 - internal/plugin/grpc_provider.go | 2 +- internal/plugin/grpc_provisioner.go | 2 +- internal/plugin6/grpc_provider.go | 2 +- internal/states/remote/state.go | 3 +-- internal/states/statemgr/filesystem.go | 10 ++++----- internal/tofu/context.go | 2 +- internal/tofu/eval_context_builtin.go | 21 +++++++++++-------- internal/tofu/evaluate.go | 4 ++-- internal/tofu/graph_walk_context.go | 19 ++++++++++------- internal/tofu/opentf_test.go | 3 +-- 26 files changed, 61 insertions(+), 70 deletions(-) diff --git a/internal/backend/remote-state/consul/client.go b/internal/backend/remote-state/consul/client.go index 3ac643ab71..759af4db2a 100644 --- a/internal/backend/remote-state/consul/client.go +++ b/internal/backend/remote-state/consul/client.go @@ -42,11 +42,11 @@ var lostLockErr = errors.New("consul lock was lost") // RemoteClient is a remote client that stores data in Consul. type RemoteClient struct { - Client *consulapi.Client - Path string - GZip bool + Path string + GZip bool - mu sync.Mutex + mu sync.Mutex + Client *consulapi.Client // lockState is true if we're using locks lockState bool diff --git a/internal/checks/state.go b/internal/checks/state.go index cfa157dcb4..2b63981263 100644 --- a/internal/checks/state.go +++ b/internal/checks/state.go @@ -35,8 +35,7 @@ import ( // This container type is concurrency-safe for both reads and writes through // its various methods. type State struct { - mu sync.Mutex - + mu sync.Mutex statuses addrs.Map[addrs.ConfigCheckable, *configCheckableState] failureMsgs addrs.Map[addrs.CheckRule, string] } diff --git a/internal/cloud/state.go b/internal/cloud/state.go index 9cd2ee0d8b..92b32a5005 100644 --- a/internal/cloud/state.go +++ b/internal/cloud/state.go @@ -47,8 +47,6 @@ const ( // local caching so every persist will go to the remote storage and local // writes will go to memory. type State struct { - mu sync.Mutex - // We track two pieces of meta data in addition to the state itself: // // lineage - the state's unique ID @@ -60,6 +58,7 @@ type State struct { // state has changed from an existing state we read in. lineage, readLineage string serial, readSerial uint64 + mu sync.Mutex state, readState *states.State disableLocks bool tfeClient *tfe.Client diff --git a/internal/cloud/tfe_client_mock.go b/internal/cloud/tfe_client_mock.go index d39c53c026..5b8b00f00b 100644 --- a/internal/cloud/tfe_client_mock.go +++ b/internal/cloud/tfe_client_mock.go @@ -1052,7 +1052,6 @@ func (m *MockProjects) Delete(ctx context.Context, projectID string) error { type MockRuns struct { sync.Mutex - client *MockClient Runs map[string]*tfe.Run workspaces map[string][]*tfe.Run diff --git a/internal/command/clistate/local_state.go b/internal/command/clistate/local_state.go index 5b2fbff86a..fffcf65a19 100644 --- a/internal/command/clistate/local_state.go +++ b/internal/command/clistate/local_state.go @@ -22,8 +22,6 @@ import ( // LocalState manages a state storage that is local to the filesystem. type LocalState struct { - mu sync.Mutex - // Path is the path to read the state from. PathOut is the path to // write the state to. If PathOut is not specified, Path will be used. // If PathOut already exists, it will be overwritten. @@ -43,6 +41,7 @@ type LocalState struct { // hurt to remove file we never wrote to. created bool + mu sync.Mutex state *tofu.State readState *tofu.State written bool diff --git a/internal/command/clistate/state.go b/internal/command/clistate/state.go index 979d6cb029..9d9235c899 100644 --- a/internal/command/clistate/state.go +++ b/internal/command/clistate/state.go @@ -70,9 +70,9 @@ type Locker interface { } type locker struct { - mu sync.Mutex ctx context.Context timeout time.Duration + mu sync.Mutex state statemgr.Locker view views.StateLocker lockID string diff --git a/internal/command/views/hook_count.go b/internal/command/views/hook_count.go index 2264c57d0d..8ecfd4465f 100644 --- a/internal/command/views/hook_count.go +++ b/internal/command/views/hook_count.go @@ -29,9 +29,9 @@ type countHook struct { ToRemove int ToRemoveAndAdd int + sync.Mutex pending map[string]plans.Action - sync.Mutex tofu.NilHook } diff --git a/internal/command/views/hook_json.go b/internal/command/views/hook_json.go index 37c8cd1981..4df1c61fb9 100644 --- a/internal/command/views/hook_json.go +++ b/internal/command/views/hook_json.go @@ -39,10 +39,10 @@ type jsonHook struct { view *JSONView + applyingLock sync.Mutex // Concurrent map of resource addresses to allow the sequence of pre-apply, // progress, and post-apply messages to share data about the resource - applying map[string]applyProgress - applyingLock sync.Mutex + applying map[string]applyProgress // Mockable functions for testing the progress timer goroutine timeNow func() time.Time diff --git a/internal/command/views/hook_ui.go b/internal/command/views/hook_ui.go index f74d156660..8ae8024d00 100644 --- a/internal/command/views/hook_ui.go +++ b/internal/command/views/hook_ui.go @@ -38,13 +38,13 @@ func NewUiHook(view *View) *UiHook { type UiHook struct { tofu.NilHook - view *View viewLock sync.Mutex + view *View periodicUiTimer time.Duration - resources map[string]uiResourceState resourcesLock sync.Mutex + resources map[string]uiResourceState } var _ tofu.Hook = (*UiHook)(nil) diff --git a/internal/dag/walk.go b/internal/dag/walk.go index 3a66c12900..d73575cd2f 100644 --- a/internal/dag/walk.go +++ b/internal/dag/walk.go @@ -49,15 +49,15 @@ type Walker struct { // changeLock must be held to modify any of the fields below. Only Update // should modify these fields. Modifying them outside of Update can cause // serious problems. - changeLock sync.Mutex - vertices Set - edges Set - vertexMap map[Vertex]*walkerVertex + changeLock sync.Mutex + vertices, edges Set + vertexMap map[Vertex]*walkerVertex // wait is done when all vertices have executed. It may become "undone" // if new vertices are added. wait sync.WaitGroup + diagsLock sync.Mutex // diagsMap contains the diagnostics recorded so far for execution, // and upstreamFailed contains all the vertices whose problems were // caused by upstream failures, and thus whose diagnostics should be @@ -66,7 +66,6 @@ type Walker struct { // Readers and writers of either map must hold diagsLock. diagsMap map[Vertex]tfdiags.Diagnostics upstreamFailed map[Vertex]struct{} - diagsLock sync.Mutex } func (w *Walker) init() { @@ -92,6 +91,7 @@ type walkerVertex struct { DoneCh chan struct{} CancelCh chan struct{} + DepsLock sync.Mutex // Dependency information. Any changes to any of these fields requires // holding DepsLock. // @@ -102,7 +102,6 @@ type walkerVertex struct { // DepsUpdateCh is closed when there is a new DepsCh set. DepsCh chan bool DepsUpdateCh chan struct{} - DepsLock sync.Mutex // Below is not safe to read/write in parallel. This behavior is // enforced by changes only happening in Update. Nothing else should diff --git a/internal/getproviders/memoize_source.go b/internal/getproviders/memoize_source.go index d288800283..257c924417 100644 --- a/internal/getproviders/memoize_source.go +++ b/internal/getproviders/memoize_source.go @@ -25,9 +25,9 @@ import ( // sequentially. type MemoizeSource struct { underlying Source + mu sync.Mutex availableVersions map[addrs.Provider]memoizeAvailableVersionsRet packageMetas map[memoizePackageMetaCall]memoizePackageMetaRet - mu sync.Mutex } type memoizeAvailableVersionsRet struct { diff --git a/internal/lang/scope.go b/internal/lang/scope.go index 60950c9d57..8827523162 100644 --- a/internal/lang/scope.go +++ b/internal/lang/scope.go @@ -56,8 +56,8 @@ type Scope struct { // then differ during apply. PureOnly bool - funcs map[string]function.Function funcsLock sync.Mutex + funcs map[string]function.Function // activeExperiments is an optional set of experiments that should be // considered as active in the module that this scope will be used for. diff --git a/internal/legacy/helper/schema/field_writer_map.go b/internal/legacy/helper/schema/field_writer_map.go index 1d746f3aa6..196f123071 100644 --- a/internal/legacy/helper/schema/field_writer_map.go +++ b/internal/legacy/helper/schema/field_writer_map.go @@ -17,9 +17,8 @@ import ( // MapFieldWriter writes data into a single map[string]string structure. type MapFieldWriter struct { - Schema map[string]*Schema - lock sync.Mutex + Schema map[string]*Schema result map[string]string } diff --git a/internal/legacy/helper/schema/resource_diff.go b/internal/legacy/helper/schema/resource_diff.go index a52742484f..aa97c85fa7 100644 --- a/internal/legacy/helper/schema/resource_diff.go +++ b/internal/legacy/helper/schema/resource_diff.go @@ -21,12 +21,11 @@ import ( type newValueWriter struct { *MapFieldWriter - // A list of keys that should be marked as computed. - computedKeys map[string]bool - // A lock to prevent races on writes. The underlying writer will have one as // well - this is for computed keys. lock sync.Mutex + // A list of keys that should be marked as computed. + computedKeys map[string]bool // To be used with init. once sync.Once diff --git a/internal/legacy/tofu/state.go b/internal/legacy/tofu/state.go index dea949b3c9..ca9a3d6db4 100644 --- a/internal/legacy/tofu/state.go +++ b/internal/legacy/tofu/state.go @@ -112,10 +112,9 @@ type State struct { // configuration. Backend *BackendState `json:"backend,omitempty"` + mu sync.Mutex // Modules contains all the modules in a breadth-first order Modules []*ModuleState `json:"modules"` - - mu sync.Mutex } func (s *State) Lock() { s.mu.Lock() } @@ -837,11 +836,10 @@ type RemoteState struct { // Type controls the client we use for the remote state Type string `json:"type"` + mu sync.Mutex // Config is used to store arbitrary configuration that // is type specific Config map[string]string `json:"config"` - - mu sync.Mutex } func (s *RemoteState) Lock() { s.mu.Lock() } @@ -1621,6 +1619,7 @@ type InstanceState struct { // and is only meant as a lookup mechanism for the providers. ID string `json:"id"` + mu sync.Mutex // Attributes are basic information about the resource. Any keys here // are accessible in variable format within OpenTofu configurations: // ${resourcetype.name.attribute}. @@ -1641,8 +1640,6 @@ type InstanceState struct { // Tainted is used to mark a resource for recreation. Tainted bool `json:"tainted"` - - mu sync.Mutex } func (s *InstanceState) Lock() { s.mu.Lock() } diff --git a/internal/logging/panic.go b/internal/logging/panic.go index 2e20a17081..1fd2189782 100644 --- a/internal/logging/panic.go +++ b/internal/logging/panic.go @@ -127,7 +127,6 @@ func PluginPanics() []string { // happened when a plugin suddenly terminates. type panicRecorder struct { sync.Mutex - // panics maps the plugin name to the panic output lines received from // the logger. panics map[string][]string diff --git a/internal/plugin/grpc_provider.go b/internal/plugin/grpc_provider.go index b2b231c825..682be4bace 100644 --- a/internal/plugin/grpc_provider.go +++ b/internal/plugin/grpc_provider.go @@ -70,9 +70,9 @@ type GRPCProvider struct { // plugin process ends. ctx context.Context + mu sync.Mutex // schema stores the schema for this provider. This is used to properly // serialize the requests for schemas. - mu sync.Mutex schema providers.GetProviderSchemaResponse } diff --git a/internal/plugin/grpc_provisioner.go b/internal/plugin/grpc_provisioner.go index 3a6f21a77d..e8292ea263 100644 --- a/internal/plugin/grpc_provisioner.go +++ b/internal/plugin/grpc_provisioner.go @@ -48,8 +48,8 @@ type GRPCProvisioner struct { client proto.ProvisionerClient ctx context.Context + mu sync.Mutex // Cache the schema since we need it for serialization in each method call. - mu sync.Mutex schema *configschema.Block } diff --git a/internal/plugin6/grpc_provider.go b/internal/plugin6/grpc_provider.go index 1352323e20..4c377f2e07 100644 --- a/internal/plugin6/grpc_provider.go +++ b/internal/plugin6/grpc_provider.go @@ -70,9 +70,9 @@ type GRPCProvider struct { // plugin process ends. ctx context.Context + mu sync.Mutex // schema stores the schema for this provider. This is used to properly // serialize the requests for schemas. - mu sync.Mutex schema providers.GetProviderSchemaResponse } diff --git a/internal/states/remote/state.go b/internal/states/remote/state.go index 60064a875c..3874ed061d 100644 --- a/internal/states/remote/state.go +++ b/internal/states/remote/state.go @@ -26,8 +26,6 @@ import ( // local caching so every persist will go to the remote storage and local // writes will go to memory. type State struct { - mu sync.Mutex - Client Client encryption encryption.StateEncryption @@ -43,6 +41,7 @@ type State struct { // state has changed from an existing state we read in. lineage, readLineage string serial, readSerial uint64 + mu sync.Mutex state, readState *states.State disableLocks bool diff --git a/internal/states/statemgr/filesystem.go b/internal/states/statemgr/filesystem.go index 5e69e6e113..2b92d06985 100644 --- a/internal/states/statemgr/filesystem.go +++ b/internal/states/statemgr/filesystem.go @@ -29,8 +29,6 @@ import ( // // The transient storage for Filesystem is always in-memory. type Filesystem struct { - mu sync.Mutex - // path is the location where a file will be created or replaced for // each persistent snapshot. path string @@ -59,10 +57,10 @@ type Filesystem struct { // hurt to remove file we never wrote to. created bool - file *statefile.File - readFile *statefile.File - backupFile *statefile.File - writtenBackup bool + mu sync.Mutex + file, readFile *statefile.File + backupFile *statefile.File + writtenBackup bool encryption encryption.StateEncryption } diff --git a/internal/tofu/context.go b/internal/tofu/context.go index 3f99b9825b..0c03dc1e48 100644 --- a/internal/tofu/context.go +++ b/internal/tofu/context.go @@ -85,8 +85,8 @@ type Context struct { sh *stopHook uiInput UIInput - l sync.Mutex // Lock acquired during any task parallelSem Semaphore + l sync.Mutex // Lock acquired during any task providerInputConfig map[string]map[string]cty.Value runCond *sync.Cond runContext context.Context diff --git a/internal/tofu/eval_context_builtin.go b/internal/tofu/eval_context_builtin.go index 539a233b7e..35cb36b5c4 100644 --- a/internal/tofu/eval_context_builtin.go +++ b/internal/tofu/eval_context_builtin.go @@ -52,26 +52,29 @@ type BuiltinEvalContext struct { // eval context. Evaluator *Evaluator + VariableValuesLock *sync.Mutex // VariableValues contains the variable values across all modules. This // structure is shared across the entire containing context, and so it // may be accessed only when holding VariableValuesLock. // The keys of the first level of VariableValues are the string // representations of addrs.ModuleInstance values. The second-level keys // are variable names within each module instance. - VariableValues map[string]map[string]cty.Value - VariableValuesLock *sync.Mutex + VariableValues map[string]map[string]cty.Value // Plugins is a library of plugin components (providers and provisioners) // available for use during a graph walk. Plugins *contextPlugins - Hooks []Hook - InputValue UIInput - ProviderCache map[string]providers.Interface - ProviderInputConfig map[string]map[string]cty.Value - ProviderLock *sync.Mutex - ProvisionerCache map[string]provisioners.Interface - ProvisionerLock *sync.Mutex + Hooks []Hook + InputValue UIInput + + ProviderLock *sync.Mutex + ProviderCache map[string]providers.Interface + ProviderInputConfig map[string]map[string]cty.Value + + ProvisionerLock *sync.Mutex + ProvisionerCache map[string]provisioners.Interface + ChangesValue *plans.ChangesSync StateValue *states.SyncState ChecksValue *checks.State diff --git a/internal/tofu/evaluate.go b/internal/tofu/evaluate.go index 13718c0147..fb4b322481 100644 --- a/internal/tofu/evaluate.go +++ b/internal/tofu/evaluate.go @@ -41,6 +41,7 @@ type Evaluator struct { // Config is the root node in the configuration tree. Config *configs.Config + VariableValuesLock *sync.Mutex // VariableValues is a map from variable names to their associated values, // within the module indicated by ModulePath. VariableValues is modified // concurrently, and so it must be accessed only while holding @@ -48,8 +49,7 @@ type Evaluator struct { // // The first map level is string representations of addr.ModuleInstance // values, while the second level is variable names. - VariableValues map[string]map[string]cty.Value - VariableValuesLock *sync.Mutex + VariableValues map[string]map[string]cty.Value // Plugins is the library of available plugin components (providers and // provisioners) that we have available to help us evaluate expressions diff --git a/internal/tofu/graph_walk_context.go b/internal/tofu/graph_walk_context.go index ef2bc3fbbd..750274fc72 100644 --- a/internal/tofu/graph_walk_context.go +++ b/internal/tofu/graph_walk_context.go @@ -51,15 +51,18 @@ type ContextGraphWalker struct { // is in progress. NonFatalDiagnostics tfdiags.Diagnostics - once sync.Once - contexts map[string]*BuiltinEvalContext - contextLock sync.Mutex - variableValues map[string]map[string]cty.Value + once sync.Once + contextLock sync.Mutex + contexts map[string]*BuiltinEvalContext + variableValuesLock sync.Mutex - providerCache map[string]providers.Interface - providerLock sync.Mutex - provisionerCache map[string]provisioners.Interface - provisionerLock sync.Mutex + variableValues map[string]map[string]cty.Value + + providerLock sync.Mutex + providerCache map[string]providers.Interface + + provisionerLock sync.Mutex + provisionerCache map[string]provisioners.Interface } func (w *ContextGraphWalker) EnterPath(path addrs.ModuleInstance) EvalContext { diff --git a/internal/tofu/opentf_test.go b/internal/tofu/opentf_test.go index 46f7b4e613..17b8c65467 100644 --- a/internal/tofu/opentf_test.go +++ b/internal/tofu/opentf_test.go @@ -250,11 +250,10 @@ type HookRecordApplyOrder struct { Active bool + l sync.Mutex IDs []string States []cty.Value Diffs []*plans.Change - - l sync.Mutex } func (h *HookRecordApplyOrder) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, action plans.Action, priorState, plannedNewState cty.Value) (HookAction, error) {