Fix missing unlock in uniquequeue #9790

zeripath · 2020-01-15T21:23:52Z

zeripath commented

2020-01-15 21:23:52 +00:00

Fixes a missing unlock in uniquequeue that will cause a deadlock.

Fix #9667
Likely Fixes #9759

Fixes a missing unlock in uniquequeue that will cause a deadlock. Fix #9667 Likely Fixes #9759

davidsvantesson (Migrated from github.com) reviewed 2020-01-15 21:28:38 +00:00

modules/sync/unique_queue.go

						
				@ -82,6 +82,7 @@ func (q *UniqueQueue) AddFunc(id interface{}, fn func()) {

					idStr := com.ToStr(id)

					q.table.lock.Lock()

					if _, ok := q.table.pool[idStr]; ok {

						q.table.lock.Unlock()

davidsvantesson (Migrated from github.com) commented

2020-01-15 21:28:37 +00:00

Can use defer instead?

zeripath reviewed 2020-01-15 21:29:58 +00:00

modules/sync/unique_queue.go

						
				@ -82,6 +82,7 @@ func (q *UniqueQueue) AddFunc(id interface{}, fn func()) {

					idStr := com.ToStr(id)

					q.table.lock.Lock()

					if _, ok := q.table.pool[idStr]; ok {

						q.table.lock.Unlock()

zeripath commented

2020-01-15 21:29:58 +00:00

nope you want to unlock before you push to the channel.

davidsvantesson (Migrated from github.com) approved these changes 2020-01-15 21:31:18 +00:00

zeripath reviewed 2020-01-15 21:31:28 +00:00

modules/sync/unique_queue.go

						
				@ -82,6 +82,7 @@ func (q *UniqueQueue) AddFunc(id interface{}, fn func()) {

					idStr := com.ToStr(id)

					q.table.lock.Lock()

					if _, ok := q.table.pool[idStr]; ok {

						q.table.lock.Unlock()

zeripath commented

2020-01-15 21:31:28 +00:00

the only issue is if you panic in fn() then you might not get unlocked...

zeripath reviewed 2020-01-15 21:33:49 +00:00

modules/sync/unique_queue.go

						
				@ -82,6 +82,7 @@ func (q *UniqueQueue) AddFunc(id interface{}, fn func()) {

					idStr := com.ToStr(id)

					q.table.lock.Lock()

					if _, ok := q.table.pool[idStr]; ok {

						q.table.lock.Unlock()

zeripath commented

2020-01-15 21:33:49 +00:00

really not sure what you can do to protect against that.

zeripath reviewed 2020-01-15 21:46:52 +00:00

modules/sync/unique_queue.go

						
				@ -82,6 +82,7 @@ func (q *UniqueQueue) AddFunc(id interface{}, fn func()) {

					idStr := com.ToStr(id)

					q.table.lock.Lock()

					if _, ok := q.table.pool[idStr]; ok {

						q.table.lock.Unlock()

zeripath commented

2020-01-15 21:46:51 +00:00

I suppose this would cope:

func (q *UniqueQueue) AddFunc(id interface{}, fn func()) {
	idStr := com.ToStr(id)
	q.table.lock.Lock()
	locked := true
	defer func() {
		if locked {
			q.table.lock.Unlock()
		}
	}()
	if _, ok := q.table.pool[idStr]; ok {
		return
	}
	q.table.pool[idStr] = struct{}{}
	if fn != nil {
		fn()
	}
	locked = false
	q.table.lock.Unlock()
	select {
	case <-q.closed:
		return
	case q.queue <- idStr:
		return
	}
}

I suppose this would cope: ```go func (q *UniqueQueue) AddFunc(id interface{}, fn func()) { idStr := com.ToStr(id) q.table.lock.Lock() locked := true defer func() { if locked { q.table.lock.Unlock() } }() if _, ok := q.table.pool[idStr]; ok { return } q.table.pool[idStr] = struct{}{} if fn != nil { fn() } locked = false q.table.lock.Unlock() select { case <-q.closed: return case q.queue <- idStr: return } } ```