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

lease: guard 'Lease.itemSet' from concurrent writes #7457

Merged
merged 1 commit into from
Mar 8, 2017

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Mar 8, 2017

Fix #7448.

Affected if etcd builds with Go 1.8+.

lease/lessor.go Outdated
@@ -515,6 +520,9 @@ func (l *Lease) expired() bool {
}

func (l *Lease) persistTo(b backend.Backend) {
l.mu.RLock()
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't need to be acquired since not touching itemSet?

lease/lessor.go Outdated
@@ -530,12 +538,15 @@ func (l *Lease) persistTo(b backend.Backend) {

// TTL returns the TTL of the Lease.
func (l *Lease) TTL() int64 {
return l.ttl
l.mu.RLock()
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't need to be acquired since not touching itemSet?

lease/lessor.go Outdated
@@ -501,6 +505,7 @@ func (le *lessor) initAndRecover() {
}

type Lease struct {
mu sync.RWMutex
Copy link
Contributor

Choose a reason for hiding this comment

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

// mu protects concurrent accesses to itemSet?

}

<-donec
}
Copy link
Contributor

Choose a reason for hiding this comment

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

wg.Wait()?

lease/lessor.go Outdated
}

// refresh refreshes the expiry of the lease.
func (l *Lease) refresh(extend time.Duration) {
t := monotime.Now().Add(extend + time.Duration(l.ttl)*time.Second)
t := monotime.Now().Add(extend + time.Duration(l.TTL())*time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

can stay the same?

@gyuho
Copy link
Contributor Author

gyuho commented Mar 8, 2017

@heyitsanthony Yeah fixed it to only protect itemSet. PTAL. Thanks!

@heyitsanthony
Copy link
Contributor

lgtm. Thanks!

@fanminshi
Copy link
Member

change this

func (le *lessor) Revoke(id LeaseID) error {
...
// not under le.mu at here.
// so detach() can operate on l.itemset at the same time revoke() loops l.itemSet. 
keys := make([]string, 0, len(l.itemSet))
	for item := range l.itemSet {
		keys = append(keys, item.Key)
	}
...

https://github.com/gyuho/etcd/blob/508c83becef22fc4d08e8c96039d5ac06ded749f/lease/lessor.go#L255-L258

to

keys := l.Keys()

Fix etcd-io#7448.

Affected if etcd builds with Go 1.8+.

Signed-off-by: Gyu-Ho Lee <[email protected]>
@fanminshi
Copy link
Member

lgtm thanks!

@gyuho gyuho merged commit 1bcbd82 into etcd-io:master Mar 8, 2017
@gyuho gyuho deleted the lease-guard branch March 8, 2017 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants