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.

Sign up for GitHub

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

Jump to bottom

fix conflict between handleReload and antiEntropy critical sections #1235

Merged
merged 3 commits into from
Sep 17, 2015
Merged

fix conflict between handleReload and antiEntropy critical sections #1235

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
24bc17e
failing test showing that nested Pause()/Resume() release too early
wuub Sep 11, 2015
b014c0f
make Pause()/Resume()/isPaused() behave more like a semaphore
wuub Sep 11, 2015
c4537ed
panic when unbalanced localState.Resume() is detected
wuub Sep 17, 2015
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
6 changes: 3 additions & 3 deletions command/agent/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,18 +109,18 @@ func (l *localState) ConsulServerUp() {
// Pause is used to pause state synchronization, this can be
// used to make batch changes
func (l *localState) Pause() {
atomic.StoreInt32(&l.paused, 1)
atomic.AddInt32(&l.paused, 1)
}

// Resume is used to resume state synchronization
func (l *localState) Resume() {
atomic.StoreInt32(&l.paused, 0)
atomic.AddInt32(&l.paused, -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this returns the new value, I think I would put the panic in here to immediately catch any mismatch of these if it underflows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Proper check added in the c4537ed.
I'm more concerned about overflows TBH, there is no way to detect them at the moment and one missing defer to .Resume() can block antiEntropy forever.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about this as well. We could enforce an arbitrary upper limit on the semaphore to catch the case where someone is walking it up, which is probably the most likely programming error, but it won't catch an oddball bad path that doesn't get called often.

l.changeMade()
}

// isPaused is used to check if we are paused
func (l *localState) isPaused() bool {
return atomic.LoadInt32(&l.paused) == 1
return atomic.LoadInt32(&l.paused) > 0
}

// ServiceToken returns the configured ACL token for the given
Expand Down
23 changes: 23 additions & 0 deletions command/agent/local_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,29 @@ func TestAgent_checkTokens(t *testing.T) {
}
}

func TestAgent_nestedPauseResume(t *testing.T) {
l := new(localState)
if l.isPaused() != false {
t.Fatal("localState should be unPaused after init")
}
l.Pause()
if l.isPaused() != true {
t.Fatal("localState should be Paused after first call to Pause()")
}
l.Pause()
if l.isPaused() != true {
t.Fatal("localState should STILL be Paused after second call to Pause()")
}
l.Resume()
if l.isPaused() != true {
t.Fatal("localState should STILL be Paused after FIRST call to Resume()")
}
l.Resume()
if l.isPaused() != false {
t.Fatal("localState should NOT be Paused after SECOND call to Resume()")
}
}

var testRegisterRules = `
service "api" {
policy = "write"
Expand Down