From e8119cced39bdd9edf0944f35a8c980336c2c6d1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 15 May 2024 14:08:57 -0400 Subject: [PATCH] remove extra state.DeepCopy from updateStateHook All production state implementations copy the given state when storing it to temporary or permanent storage, either explicitly with DeepCopy, or implicitly by serializing the state. Since `updateStateHook` is called with every state change, a second call to `DeepCopy` with that hook doubles the overhead of `PostStateUpdate`. Since all Hook implementations which handle `PostStateUpdate` (which is exactly 1 in non-test code) only call state methods that eventually copy the state, we can remove the extra copy from the `updateStateHook` itself. The state was already locked for the duration of the `PostStateUpdate` call previously, so no additional delay should result. For consistency this also updates the documentation for `PostStateUpdate` to record the new caller and callee expectations based on the existing implementation. The Hook interface predated the `statemgr` which provides a better interface for synchronizing state access and therefor required extra copying at that time, while now we can more easily declare that the method requires concurrent access be locked for the duration of the call. --- internal/terraform/hook.go | 8 +++++--- internal/terraform/update_state_hook.go | 9 +++------ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/internal/terraform/hook.go b/internal/terraform/hook.go index 2e457f2814..f7dbd6af04 100644 --- a/internal/terraform/hook.go +++ b/internal/terraform/hook.go @@ -114,9 +114,11 @@ type Hook interface { // function is called. Stopping() - // PostStateUpdate is called each time the state is updated. It receives - // a deep copy of the state, which it may therefore access freely without - // any need for locks to protect from concurrent writes from the caller. + // PostStateUpdate is called each time the state is updated. The caller must + // coordinate a lock for the state if necessary, such that the Hook may + // access it freely without any need for additional locks to protect from + // concurrent writes. Implementations which modify or retain the state after + // the call has returned must copy the state. PostStateUpdate(new *states.State) (HookAction, error) } diff --git a/internal/terraform/update_state_hook.go b/internal/terraform/update_state_hook.go index 3aa551df50..35dc4d82cd 100644 --- a/internal/terraform/update_state_hook.go +++ b/internal/terraform/update_state_hook.go @@ -5,13 +5,10 @@ package terraform // updateStateHook calls the PostStateUpdate hook with the current state. func updateStateHook(ctx EvalContext) error { - // In principle we could grab the lock here just long enough to take a - // deep copy and then pass that to our hooks below, but we'll instead - // hold the hook for the duration to avoid the potential confusing - // situation of us racing to call PostStateUpdate concurrently with - // different state snapshots. + // PostStateUpdate requires that the state be locked and safe to read for + // the duration of the call. stateSync := ctx.State() - state := stateSync.Lock().DeepCopy() + state := stateSync.Lock() defer stateSync.Unlock() // Call the hook