Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't open a new file descriptor for a locked state. #17636

Merged
merged 3 commits into from
Mar 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion state/local.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,20 @@ func (s *LocalState) RefreshState() error {
s.mu.Lock()
defer s.mu.Unlock()

if s.PathOut == "" {
s.PathOut = s.Path
}

var reader io.Reader
if !s.written {

// The s.Path file is only OK to read if we have not written any state out
// (in which case the same state needs to be read in), and no state output file
// has been opened (possibly via a lock) or the input path is different
// than the output path.
// This is important for Windows, as if the input file is the same as the
// output file, and the output file has been locked already, we can't open
// the file again.
if !s.written && (s.stateFileOut == nil || s.Path != s.PathOut) {
// we haven't written a state file yet, so load from Path
f, err := os.Open(s.Path)
if err != nil {
Expand Down
39 changes: 39 additions & 0 deletions state/local_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,3 +166,42 @@ func testLocalState(t *testing.T) *LocalState {

return ls
}

// Make sure we can refresh while the state is locked
func TestLocalState_refreshWhileLocked(t *testing.T) {
f, err := ioutil.TempFile("", "tf")
if err != nil {
t.Fatalf("err: %s", err)
}

err = terraform.WriteState(TestStateInitial(), f)
f.Close()
if err != nil {
t.Fatalf("err: %s", err)
}

s := &LocalState{Path: f.Name()}
defer os.Remove(s.Path)

// lock first
info := NewLockInfo()
info.Operation = "test"
lockID, err := s.Lock(info)
if err != nil {
t.Fatal(err)
}
defer func() {
if err := s.Unlock(lockID); err != nil {
t.Fatal(err)
}
}()

if err := s.RefreshState(); err != nil {
t.Fatal(err)
}

readState := s.State()
if readState == nil || readState.Lineage == "" {
t.Fatal("missing state")
}
}
15 changes: 12 additions & 3 deletions terraform/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"io"
"io/ioutil"
"log"
"os"
"reflect"
"sort"
"strconv"
Expand Down Expand Up @@ -1876,11 +1877,19 @@ var ErrNoState = errors.New("no state")
// ReadState reads a state structure out of a reader in the format that
// was written by WriteState.
func ReadState(src io.Reader) (*State, error) {
// check for a nil file specifically, since that produces a platform
// specific error if we try to use it in a bufio.Reader.
if f, ok := src.(*os.File); ok && f == nil {
return nil, ErrNoState
}

buf := bufio.NewReader(src)

if _, err := buf.Peek(1); err != nil {
// the error is either io.EOF or "invalid argument", and both are from
// an empty state.
return nil, ErrNoState
if err == io.EOF {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removed comment here talks about both io.EOF and "invalid argument". Is the "invalid argument" case taken care of by the nil check above, or taken care of some other way here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yes, "invalid argument" is only expected when we have a nil *os.File. At one point that was ok to use for ReadState to indicate the absence of state. I'd like to not have that at all, but that takes some more serious auditing to make sure we don't have any more code paths passing it in.

return nil, ErrNoState
}
return nil, err
}

if err := testForV0State(buf); err != nil {
Expand Down