-
-
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
That's the races out of our test suite! #5384
base: develop
Are you sure you want to change the base?
Changes from all commits
8412c9d
d35da3e
5ae7d16
d28e1c5
145dd07
89f5f20
b0e5c64
08d798a
00d741f
4c0458c
7d5fb07
367a6a6
895953c
13d1f77
4713739
a0ecb94
98dc17d
b97878b
f3fbe55
e05f5ed
05919eb
06b616b
d4f57b7
6d8675d
ddacc2c
fb997d8
afb5db5
52a15d9
a446af3
154d1e6
c2f8169
2b9f46d
b37b4a5
df498a6
49cbf18
190d747
f04b0c9
6cec612
1ae8045
bcb6742
15b454e
7dbcac0
62d9f9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,15 +50,19 @@ func (p *preferences) forceImmediateSave() { | |
func (p *preferences) resetSavedRecently() { | ||
go func() { | ||
time.Sleep(time.Millisecond * 100) // writes are not always atomic. 10ms worked, 100 is safer. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I saw the message earlier but this still makes me kind of worried. Are we sure that the previous reported bug about writes not being atomic just weren't a race? Are writes not atomic or are we just missing locking between this and the place where it is written? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Go, calling As long as we are not reading and writing from different threads, I don't think atomic writes should matter much. |
||
p.prefLock.Lock() | ||
p.savedRecently = false | ||
changedDuringSaving := p.changedDuringSaving | ||
p.changedDuringSaving = false | ||
p.prefLock.Unlock() | ||
|
||
if changedDuringSaving { | ||
p.save() | ||
} | ||
// For test reasons we need to use current app not what we were initialised with as they can differ | ||
fyne.CurrentApp().Driver().CallFromGoroutine(func() { | ||
p.prefLock.Lock() | ||
p.savedRecently = false | ||
changedDuringSaving := p.changedDuringSaving | ||
p.changedDuringSaving = false | ||
p.prefLock.Unlock() | ||
|
||
if changedDuringSaving { | ||
p.save() | ||
} | ||
}) | ||
}() | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we ought to use the new
test.NewTempApp()
in all these new cases?