Deadlock from acquiring read lock twice

[williambrode]

Checklist

• I have searched the issue tracker for open issues that relate to the same problem, before opening a new one.
• This issue only relates to a single bug. I will open new issues for any other problems.

Describe the bug

I found a very similar issue in #2348 . The same underlying issue is happening for (c *checkRenderer) updateResource() and (c *checkRenderer) updateFocusIndicator() which are run inside (c *checkRenderer) Refresh(). I just hit this deadlock during (c *checkRenderer) updateResource(). The Read lock can't be acquired recursively since it may have to wait on a write lock from another thread. Thread 1 has a read lock, can't acquire a second read lock, and can't release the first read lock. Thread 2 is trying to acquire a write lock.

Here is the flow for the issue:

func (c *checkRenderer) Refresh() {
	c.check.propertyLock.RLock() <------ Takes lock
	c.applyTheme()
	c.updateLabel()
	c.updateResource() <----
	c.updateFocusIndicator()
	c.check.propertyLock.RUnlock()
	canvas.Refresh(c.check.super())
}


func (c *checkRenderer) updateResource() {
	res := theme.CheckButtonIcon()
	if c.check.Checked {
		res = theme.NewPrimaryThemedResource(theme.CheckButtonCheckedIcon())
	}
	if c.check.Disabled() { <----
		if c.check.Checked {
			res = theme.NewDisabledResource(theme.CheckButtonCheckedIcon())
		} else {
			res = theme.NewDisabledResource(res)
		}
	}
	c.icon.Resource = res
}


// Disabled returns true if this widget is currently disabled or false if it can currently be interacted with.
func (w *DisableableWidget) Disabled() bool {
	w.propertyLock.RLock() <------  Tries to lock again
	defer w.propertyLock.RUnlock()

	return w.disabled
}

How to reproduce

I don't know how to reproduce - was clicking around a bunch in my app. But the code is clearly wrong because you can't take a read lock in the same thread twice.

Screenshots

No response

Example code

Don't know what code will cause it.

Fyne version

2.2.3

Go compiler version

1.19.1

Operating system

Windows

Operating system version

Windows 10

Additional Information

No response

[andydotxyz]

Fixed on develop :)

[williambrode]

Thanks for fixing it! I think updateFocusIndicator will have the same problem? Its called after updateResource and also calls if c.check.Disabled() {

[williambrode]

@andydotxyz Any thoughts on my comment about updateFocusIndicator ? I also just opened another bug for the same kind of problem: #3886 . Wondering if there is a pattern here of read locks being taken at too many different levels.

[andydotxyz]

Opening a new issue is the right pattern - try not to bring up new issues on closed PRs and tickets.
You are probably right on there being multiple places. If the fix is simple or a repeat of the above you could also open a PR instead of a ticket I guess?

In general private methods assume a lock, but public API must attain the lock if required...