Wait for goroutines to finish when firing a change to an app preferences #2246

s77rt · 2021-05-15T19:02:52Z

Description:

Mainly there was two problems:

the first and probably the main bug is that sometimes the app closes too early without waiting for the goroutines that were called in fireChange() and one of those goroutines is the one that calls save() an easy fix was to implement the sync.WaitGroup functionality which i expected to work just fine, yet it didn't due to
the second issue which was a misuse of the lockers, in lines 70 and 71 are using Lock() and after that there are goroutines that are expected to read the same struct, well they won't be able to because Lock() only allows one goroutine (read/write) at a time and in fact the goroutines functions that were thought are being executed were actually not, they were just in stuck position waiting for the fireChange() to return (and to trigger it's defer function to Unlock()) so all those go l() were only waiting for fireChange() to return then get executed, well after the fix (1) the function will never return because it's waiting for the goroutines functions which will never work either because they are waiting for fireChange() ,,, deadlock ; the fix was pretty clear is to use RLock() instead of Lock() that way, where multiple goroutines can read(but not write) the struct and the fireChange() will only returns after all the goroutines are done and only after that the app will close

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass. (except TestFileIcon(?))

Jacalz · 2021-05-15T20:20:32Z

Just a minor thought not related to the contents. Can you please name the title of the PR (this and in the future) to what it does and not just what it fixes? It is very hard to glance over numbers and remember what it does.

Jacalz

Just some small suggestions.

internal/preferences.go

s77rt · 2021-05-15T20:40:33Z

Just a minor thought not related to the contents. Can you please name the title of the PR (this and in the future) to what it does and not just what it fixes? It is very hard to glance over numbers and remember what it does.

ah sorry for that, the title has been changed

Jacalz

This looks good to me. I'll let someone else look on it as well though because I'm away from my computer and can't test it locally.

internal/preferences.go

andydotxyz · 2021-05-17T08:19:18Z

When running this I get deadlocks at runtime in an app that leans heavily on preferences. You can check out https://github.com/fynelabs/notes, when running (after a few edits) I see:

❯ go run .
2021/05/17 09:17:44 Fyne error:  Preferences load error:
2021/05/17 09:17:44   Cause: EOF
2021/05/17 09:17:44   At: /Users/andy/Code/Fyne/fyne/app/preferences.go:70
2021/05/17 09:17:49 Fyne error:  Preferences load error:
2021/05/17 09:17:49   Cause: EOF
2021/05/17 09:17:49   At: /Users/andy/Code/Fyne/fyne/app/preferences.go:70
2021/05/17 09:17:57 Fyne error:  Preferences load error:
2021/05/17 09:17:57   Cause: EOF
2021/05/17 09:17:57   At: /Users/andy/Code/Fyne/fyne/app/preferences.go:70

That was when trying to quit the app, the window disappeared but the process did not exit.
Could it be related? It does not happen every time but I am pretty sure I have not seen it before.

Oh, apologies I should have said earlier, thanks for this - I had wondered if something was incorrect as I realised that sometimes this notes app did not save the final character when making notes and exiting!

waitGroup *sync.WaitGroup to wg *sync.WaitGroup

…d so it's easier to debug

s77rt · 2021-05-17T15:17:00Z

When running this I get deadlocks at runtime in an app that leans heavily on preferences. You can check out https://github.com/fynelabs/notes, when running (after a few edits) I see:
❯ go run .
2021/05/17 09:17:44 Fyne error:  Preferences load error:
2021/05/17 09:17:44   Cause: EOF
2021/05/17 09:17:44   At: /Users/andy/Code/Fyne/fyne/app/preferences.go:70
2021/05/17 09:17:49 Fyne error:  Preferences load error:
2021/05/17 09:17:49   Cause: EOF
2021/05/17 09:17:49   At: /Users/andy/Code/Fyne/fyne/app/preferences.go:70
2021/05/17 09:17:57 Fyne error:  Preferences load error:
2021/05/17 09:17:57   Cause: EOF
2021/05/17 09:17:57   At: /Users/andy/Code/Fyne/fyne/app/preferences.go:70
That was when trying to quit the app, the window disappeared but the process did not exit.
Could it be related? It does not happen every time but I am pretty sure I have not seen it before.

Oh, apologies I should have said earlier, thanks for this - I had wondered if something was incorrect as I realised that sometimes this notes app did not save the final character when making notes and exiting!

I have tried your app and i can't replicate the issue, though sometimes the app does not save the whole string (as you said some chars are left) (some saves are not being saved?)

can you please provide more info about how to replicate this?

andydotxyz

Thanks the code looks good now, but I missed that line of documentation isn't particularly clear.

internal/preferences.go

andydotxyz

Nice thanks

Jacalz · 2021-05-24T11:21:57Z

I will have a look at this again later today. Want to make sure that it works for my use cases before we merge it :)

Jacalz

Works perfectly fine in my testing. Nice work!

…ces (#2246) Mainly there was two problems: - the first and probably the main bug is that sometimes the app closes too early without waiting for the goroutines that were called in [fireChange()](https://github.com/fyne-io/fyne/blob/3814a66f75ff5d24be6fa107f1dfb9c4faa859b1/internal/preferences.go#L69) and one of those goroutines is the one that calls [save()](https://github.com/fyne-io/fyne/blob/3814a66f75ff5d24be6fa107f1dfb9c4faa859b1/app/preferences.go#L35) an easy fix was to implement the sync.WaitGroup functionality which i expected to work just fine, yet it didn't due to - the second issue which was a misuse of the lockers, in lines [70 and 71](https://github.com/fyne-io/fyne/blob/3814a66f75ff5d24be6fa107f1dfb9c4faa859b1/internal/preferences.go#L70-L71) are using Lock() and after that there are goroutines that are expected to read the same struct, well they won't be able to because Lock() only allows one goroutine (read/write) at a time and in fact the goroutines functions that were thought are being executed were actually not, they were just in stuck position waiting for the fireChange() to return (and to trigger it's defer function to Unlock()) so all those go l() were only waiting for fireChange() to return then get executed, well after the fix (1) the function will never return because it's waiting for the goroutines functions which will never work either because they are waiting for fireChange() ,,, deadlock ; the fix was pretty clear is to use RLock() instead of Lock() that way, where multiple goroutines can read(but not write) the struct and the fireChange() will only returns after all the goroutines are done and only after that the app will close Fixes #2241

fixes fyne-io#2241

4ea39dc

Jacalz reviewed May 15, 2021

View reviewed changes

internal/preferences.go Outdated Show resolved Hide resolved

internal/preferences.go Outdated Show resolved Hide resolved

s77rt changed the title ~~fixes #2241~~ Wait for goroutines to finish when firing a change to an app preferences May 15, 2021

clearer code to create a new preferences implementation stored in memory

87c93cf

s77rt requested a review from Jacalz May 15, 2021 21:02

used goimports for better code formating

f77a96b

Jacalz previously approved these changes May 16, 2021

View reviewed changes

andydotxyz requested changes May 17, 2021

View reviewed changes

internal/preferences.go Outdated Show resolved Hide resolved

internal/preferences.go Outdated Show resolved Hide resolved

internal/preferences.go Outdated Show resolved Hide resolved

changed variable name so it's not the same as it's type:

18cceb5

waitGroup *sync.WaitGroup to wg *sync.WaitGroup

s77rt dismissed Jacalz’s stale review via 18cceb5 May 17, 2021 14:40

s77rt added 2 commits May 17, 2021 15:50

enhanced code that wg.Add(), wg.Done() and wg.Wait() are not separate…

7e93970

…d so it's easier to debug

fixed: loop variable l captured by func literal

b55449b

andydotxyz requested changes May 19, 2021

View reviewed changes

internal/preferences.go Outdated Show resolved Hide resolved

re-typed a comment

d025ffd

s77rt requested a review from andydotxyz May 21, 2021 12:11

andydotxyz approved these changes May 24, 2021

View reviewed changes

Jacalz self-requested a review May 24, 2021 11:19

Jacalz approved these changes May 24, 2021

View reviewed changes

Jacalz merged commit 0921509 into fyne-io:develop May 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait for goroutines to finish when firing a change to an app preferences #2246

Wait for goroutines to finish when firing a change to an app preferences #2246

s77rt commented May 15, 2021 •

edited

Jacalz commented May 15, 2021

Jacalz left a comment

s77rt commented May 15, 2021

Jacalz left a comment

andydotxyz commented May 17, 2021 •

edited

s77rt commented May 17, 2021 •

edited

andydotxyz left a comment

andydotxyz left a comment

Jacalz commented May 24, 2021

Jacalz left a comment

Wait for goroutines to finish when firing a change to an app preferences #2246

Wait for goroutines to finish when firing a change to an app preferences #2246

Conversation

s77rt commented May 15, 2021 • edited

Description:

Checklist:

Jacalz commented May 15, 2021

Jacalz left a comment

Choose a reason for hiding this comment

s77rt commented May 15, 2021

Jacalz left a comment

Choose a reason for hiding this comment

andydotxyz commented May 17, 2021 • edited

s77rt commented May 17, 2021 • edited

andydotxyz left a comment

Choose a reason for hiding this comment

andydotxyz left a comment

Choose a reason for hiding this comment

Jacalz commented May 24, 2021

Jacalz left a comment

Choose a reason for hiding this comment

s77rt commented May 15, 2021 •

edited

andydotxyz commented May 17, 2021 •

edited

s77rt commented May 17, 2021 •

edited