From a42ebe389ccfdc3916bf3f13da3ca649fc1d45b3 Mon Sep 17 00:00:00 2001
From: Martin Atkins <mart@degeneration.co.uk>
Date: Wed, 7 Jun 2017 16:13:08 -0700
Subject: [PATCH] Revert "have StateHook periodically PersistState"

This reverts commit b73d03776163466e6ed9a11d08670d78f408dd46.

This commit seems to have introduced a race condition where we can
concurrently keep updating state after we've checked if we need to
increase the serial, and thus end up writing partial changes
to the state backend.

In the case of Terraform Enterprise, this fails altogether because
of the state hash consistency check it does.
---
 backend/local/hook_state.go      | 26 ---------
 backend/local/hook_state_test.go | 97 --------------------------------
 2 files changed, 123 deletions(-)

diff --git a/backend/local/hook_state.go b/backend/local/hook_state.go
index 62e33416e..5483c4344 100644
--- a/backend/local/hook_state.go
+++ b/backend/local/hook_state.go
@@ -2,27 +2,17 @@ package local
 
 import (
 	"sync"
-	"time"
 
 	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/terraform"
 )
 
-// interval between forced PersistState calls by StateHook
-const persistStateHookInterval = 10 * time.Second
-
 // StateHook is a hook that continuously updates the state by calling
 // WriteState on a state.State.
 type StateHook struct {
 	terraform.NilHook
 	sync.Mutex
 
-	// lastPersist is the time of the last call to PersistState, for periodic
-	// updates to remote state. PostStateUpdate will force a call PersistState
-	// if it has been more that persistStateHookInterval since the last call to
-	// PersistState.
-	lastPersist time.Time
-
 	State state.State
 }
 
@@ -36,24 +26,8 @@ func (h *StateHook) PostStateUpdate(
 		if err := h.State.WriteState(s); err != nil {
 			return terraform.HookActionHalt, err
 		}
-
-		// periodically persist the state
-		if time.Since(h.lastPersist) > persistStateHookInterval {
-			if err := h.persistState(); err != nil {
-				return terraform.HookActionHalt, err
-			}
-		}
 	}
 
 	// Continue forth
 	return terraform.HookActionContinue, nil
 }
-
-func (h *StateHook) persistState() error {
-	if h.State != nil {
-		err := h.State.PersistState()
-		h.lastPersist = time.Now()
-		return err
-	}
-	return nil
-}
diff --git a/backend/local/hook_state_test.go b/backend/local/hook_state_test.go
index 1614942d6..7f4d88770 100644
--- a/backend/local/hook_state_test.go
+++ b/backend/local/hook_state_test.go
@@ -1,9 +1,7 @@
 package local
 
 import (
-	"sync"
 	"testing"
-	"time"
 
 	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/terraform"
@@ -29,98 +27,3 @@ func TestStateHook(t *testing.T) {
 		t.Fatalf("bad state: %#v", is.State())
 	}
 }
-
-// testPersistState stores the state on WriteState, and
-type testPersistState struct {
-	*state.InmemState
-
-	mu        sync.Mutex
-	persisted bool
-}
-
-func (s *testPersistState) WriteState(state *terraform.State) error {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	s.persisted = false
-	return s.InmemState.WriteState(state)
-}
-
-func (s *testPersistState) PersistState() error {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	s.persisted = true
-	return nil
-}
-
-// verify that StateHook calls PersistState if the last call was more than
-// persistStateHookInterval
-func TestStateHookPersist(t *testing.T) {
-	is := &testPersistState{
-		InmemState: &state.InmemState{},
-	}
-	hook := &StateHook{State: is}
-
-	s := state.TestStateInitial()
-	hook.PostStateUpdate(s)
-
-	// the first call should persist, since the last time was zero
-	if !is.persisted {
-		t.Fatal("PersistState not called")
-	}
-
-	s.Serial++
-	hook.PostStateUpdate(s)
-
-	// this call should not have persisted
-	if is.persisted {
-		t.Fatal("PostStateUpdate called PersistState early")
-	}
-
-	if !is.State().Equal(s) {
-		t.Fatalf("bad state: %#v", is.State())
-	}
-
-	// set the last call back to before our interval
-	hook.lastPersist = time.Now().Add(-2 * persistStateHookInterval)
-
-	s.Serial++
-	hook.PostStateUpdate(s)
-
-	if !is.persisted {
-		t.Fatal("PersistState not called")
-	}
-
-	if !is.State().Equal(s) {
-		t.Fatalf("bad state: %#v", is.State())
-	}
-}
-
-// verify that the satet hook is safe for concurrent use
-func TestStateHookRace(t *testing.T) {
-	is := &state.InmemState{}
-	var hook terraform.Hook = &StateHook{State: is}
-
-	s := state.TestStateInitial()
-
-	var wg sync.WaitGroup
-
-	for i := 0; i < 100; i++ {
-		wg.Add(1)
-		go func() {
-			defer wg.Done()
-			action, err := hook.PostStateUpdate(s)
-			if err != nil {
-				t.Fatalf("err: %s", err)
-			}
-			if action != terraform.HookActionContinue {
-				t.Fatalf("bad: %v", action)
-			}
-			if !is.State().Equal(s) {
-				t.Fatalf("bad state: %#v", is.State())
-			}
-		}()
-	}
-	wg.Wait()
-}