-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Possible deadlocks around BaseWidget.Theme access #4902
Comments
Looks like The |
That is what was fixed in the merged PR - Visible can be called unlocked but also can be called with RLock, since it acquires RLock itself to check the widget theme (the fix was that Visible used to acquire Lock, not RLock). And the button calls updateIconAndText with the RLock (or unlocked in the constroctor), never with Lock. I think the above stacktrace relates to a race condition with ApplyThemeTo. Though I have not seen a deadlock with that stack trace in real use on desktop - the PR that was merged seems to have fixed all the deadlocking around buttons I'd seen in practice. |
Great to hear. Should this be closed then? |
Well I don't think so, since the above locked stacktrace happened with the PR applied... unless we can prove it only comes up in test code |
We probably can remove the blocker from this issue however and treat it like the other races tickets in the backlog |
Let's review with latest develop - I have not seen any deadlocks since two that were found have been fixed on the branch. |
I'm glad the deadlock has gone seemingly - what is the remaining race needing to keep this open? |
The stack trace in this ticket was from a test runner on the PR that had the button deadlock fix. So it shows there's another potential deadlock that can be triggered, though perhaps only in test code. I'd say we should keep this open until we can track it down, or prove it only comes up in tests, or we have provably eliminated all races with whatever the 2.6 threading model will be |
But the related code was then resolved with the realisation that the lock should be just read... #4901 I can't see how the stack above is possible on the current state of the code. |
The stack trace here happened with the #4901 code applied though- that was my point |
🤷 I mean if you are sure it can still be replicated then re-open - but I cannot see how it logically holds any more, and I've not seen it happen in any of the test suites local or remote since the fix landed. |
Well it can always be reopened if it happens again. It happened on the test runner for the 4901 PR so unless it was related to the lifecycle deadlock that was also fixed, then it's still lurking and could come up again, though it may be really unlikely or only relevant to tests |
Sounds like a plan |
Checklist
Describe the bug
There are at least two possible deadlocks with accessing widget themes on develop. One will (maybe?) be fixed by #4901 where buttons could deadlock when pressed, but the mobile test runner for that PR discovered another deadlock around AppTabs renderer and BaseWidget.Refresh
How to reproduce
Not sure. But here is the state of the locked goroutines from the test run:
panic: test timed out after 10m0s
goroutine 47 [running]:
testing.(*M).startAlarm.func1()
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/testing/testing.go:2036 +0x8e
created by time.goFunc
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/time/sleep.go:176 +0x32
goroutine 1 [chan receive, 9 minutes]:
testing.(*T).Run(0xc0001411e0, {0x95fb4b?, 0x4e94c5?}, 0x994be8)
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/testing/testing.go:1494 +0x37a
testing.runTests.func1(0xc0001411e0?)
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/testing/testing.go:1846 +0x6e
testing.tRunner(0xc0001411e0, 0xc00029fcd8)
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/testing/testing.go:1446 +0x10b
testing.runTests(0xc0002e40a0?, {0xcd72e0, 0x46, 0x46}, {0x7f96027c0108?, 0x40?, 0x1420f20?})
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/testing/testing.go:1844 +0x456
testing.(*M).Run(0xc0002e40a0)
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/testing/testing.go:1726 +0x5d9
main.main()
_testmain.go:187 +0x1aa
goroutine 6 [chan receive, 10 minutes]:
fyne.io/fyne/v2/test.NewApp.func1()
/home/runner/work/fyne/fyne/test/testapp.go:166 +0x45
created by fyne.io/fyne/v2/test.NewApp
/home/runner/work/fyne/fyne/test/testapp.go:164 +0x2ed
goroutine 23 [semacquire, 9 minutes]:
sync.runtime_SemacquireMutex(0x0?, 0xa0?, 0xc001cec780?)
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/runtime/sema.go:77 +0x25
sync.(*RWMutex).RLock(...)
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/sync/rwmutex.go:71
fyne.io/fyne/v2/widget.(*BaseWidget).Visible(0xc001cec780?)
/home/runner/work/fyne/fyne/widget/widget.go:85 +0x53
fyne.io/fyne/v2/widget.(*buttonRenderer).updateIconAndText(0xc000130dc0)
/home/runner/work/fyne/fyne/widget/button.go:396 +0x34
fyne.io/fyne/v2/widget.(*buttonRenderer).Refresh(0xc000130dc0)
/home/runner/work/fyne/fyne/widget/button.go:308 +0xd6
fyne.io/fyne/v2/widget.(*BaseWidget).Refresh(0xc001cec780)
/home/runner/work/fyne/fyne/widget/widget.go:137 +0xb3
fyne.io/fyne/v2.(*Container).Refresh(0xc001c46550)
/home/runner/work/fyne/fyne/container.go:117 +0xe2
fyne.io/fyne/v2/container.(*baseTabsRenderer).refresh(0xc001c19480, {0xa59500?, 0xc001cec6e0?})
/home/runner/work/fyne/fyne/container/tabs.go:480 +0x36
fyne.io/fyne/v2/container.(*appTabsRenderer).Refresh(0xc001c19480)
/home/runner/work/fyne/fyne/container/apptabs.go:318 +0x4e
fyne.io/fyne/v2/widget.(*BaseWidget).Refresh(0xc001cec6e0)
/home/runner/work/fyne/fyne/widget/widget.go:137 +0xb3
fyne.io/fyne/v2/container.(*AppTabs).SelectIndex(0xc001cec6e0, 0xa57288?)
/home/runner/work/fyne/fyne/container/apptabs.go:167 +0x37
fyne.io/fyne/v2/container.TestTab_ThemeChange(0xc001c44d00?)
/home/runner/work/fyne/fyne/container/tabs_test.go:42 +0x34f
testing.tRunner(0xc001bd2d00, 0x994be8)
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/testing/testing.go:1446 +0x10b
created by testing.(*T).Run
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/testing/testing.go:1493 +0x35f
goroutine 24 [semacquire, 9 minutes]:
sync.runtime_SemacquireMutex(0xc0020a6b00?, 0x70?, 0x0?)
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/runtime/sema.go:77 +0x25
sync.(*RWMutex).Lock(0x1?)
/home/runner/work/fyne/setup-go-faster/go/1.19.13/x64/src/sync/rwmutex.go:152 +0x71
fyne.io/fyne/v2/widget.(*BaseWidget).Refresh(0xc001cec780)
/home/runner/work/fyne/fyne/widget/widget.go:132 +0x66
fyne.io/fyne/v2/internal/app.ApplyThemeTo({0xa59b60?, 0xc001cec780?}, {0xa5bbf8?, 0xc0020a6b00?})
/home/runner/work/fyne/fyne/internal/app/theme.go:29 +0x123
fyne.io/fyne/v2/internal/app.ApplyThemeTo({0xa59140, 0xc001c46550}, {0xa5bbf8, 0xc0020a6b00})
/home/runner/work/fyne/fyne/internal/app/theme.go:23 +0x245
fyne.io/fyne/v2/internal/app.ApplyThemeTo({0xa594a0, 0xc001cec6e0}, {0xa5bbf8, 0xc0020a6b00})
/home/runner/work/fyne/fyne/internal/app/theme.go:18 +0x1b3
fyne.io/fyne/v2/internal/app.ApplySettingsWithCallback({0xc001cec660?, 0xc001bd2d00?}, {0xa5b938?, 0xc001cec640?}, 0x0)
/home/runner/work/fyne/fyne/internal/app/theme.go:43 +0xca
fyne.io/fyne/v2/internal/app.ApplySettings(...)
/home/runner/work/fyne/fyne/internal/app/theme.go:35
fyne.io/fyne/v2/test.NewApp.func1()
/home/runner/work/fyne/fyne/test/testapp.go:170 +0xad
created by fyne.io/fyne/v2/test.NewApp
/home/runner/work/fyne/fyne/test/testapp.go:164 +0x2ed
FAIL fyne.io/fyne/v2/container 600.112s
Screenshots
No response
Example code
mobile tests
Fyne version
develop
Go compiler version
n/a
Operating system and version
mobile test runner GH actions
Additional Information
No response
The text was updated successfully, but these errors were encountered: