That's the races out of our test suite! #5384

andydotxyz · 2025-01-09T14:56:46Z

Working perfectly locally, let's see what CI says!

Progresses #4654

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.

…tion

Also actually run the mobile driver during tests

Fixes fyne-io#5375

andydotxyz · 2025-01-09T17:45:31Z

Oh man, so many races not detected on my local setup :(

dweymouth · 2025-01-09T18:10:06Z

InMemoryPreferences will need to avoid using a goroutine here

andydotxyz · 2025-01-09T18:12:57Z

InMemoryPreferences will need to avoid using a goroutine here

Good catch thanks. Fixed

andydotxyz · 2025-01-09T18:26:26Z

OK, some platforms are now not detecting any races, so I have enabled it on the rest and working through what I hope is the last few!!!

dweymouth · 2025-01-09T18:39:42Z

Use new RunFromGoroutine API here - https://github.com/fyne-io/fyne/blob/develop/app/settings_desktop.go#L48

coveralls · 2025-01-09T18:41:55Z

coverage: 59.759% (+0.4%) from 59.329%
when pulling 7d5fb07 on andydotxyz:fix/racedetection
into 1bfc24c on fyne-io:develop.

coveralls · 2025-01-09T18:42:03Z

coverage: 59.754% (+0.4%) from 59.329%
when pulling 367a6a6 on andydotxyz:fix/racedetection
into 1bfc24c on fyne-io:develop.

coveralls · 2025-01-09T18:42:04Z

coverage: 59.764% (+0.4%) from 59.329%
when pulling 895953c on andydotxyz:fix/racedetection
into 1bfc24c on fyne-io:develop.

coveralls · 2025-01-09T18:42:10Z

coverage: 59.256% (-0.1%) from 59.374%
when pulling 62d9f9a on andydotxyz:fix/racedetection
into f2ba049 on fyne-io:develop.

dweymouth · 2025-01-09T19:27:35Z

@Jacalz It seems we may need to remove or update the new pool_test. I think sync.Pool doesn't make a guarantee that an object released into the pool is immediately available for re-use, which is why it's failing occasionally here

andydotxyz · 2025-01-09T20:28:03Z

@Jacalz It seems we may need to remove or update the new pool_test. I think sync.Pool doesn't make a guarantee that an object released into the pool is immediately available for re-use, which is why it's failing occasionally here

Good point, the contract includes this on Get:

// Get may choose to ignore the pool and treat it as empty.
// Callers should not assume any relation between values passed to Put and
// the values returned by Get.

So I have removed the Set/Get test - but the Get on empty and the replacement New tests should both be OK.

dweymouth · 2025-01-09T21:09:13Z

This looking really good. Some greps of suspicious goroutines that I found:

fyne/internal/driver/mobile/driver.go

Line 313 in 1bfc24c

go f() // don't block main, we don't have window event queue

fyne/internal/driver/mobile/driver.go

Line 419 in 1bfc24c

go func() {

Might be nothing but just FYI that it runs in a goroutine:

fyne/app/app.go

Line 71 in 1bfc24c

go a.lifecycle.RunEventQueue()

Good finds, those will all need some work because they all result in user callbacks, or widget functions, being called directly from goroutines which is against the new model

Jacalz · 2025-01-09T21:25:08Z

That's it merged. Rebasing this should make it possible to click around in fyne_demo and test out any potential races. I think I got one when changing application scale in the settings menu earlier but it might have been fixed by this PR already, I don't know :)

…le tap mobile

andydotxyz · 2025-01-10T16:11:37Z

This sleep looks suspicous:

fyne/app/preferences.go

Line 52 in 1bfc24c

time.Sleep(time.Millisecond * 100) // writes are not always atomic. 10ms worked, 100 is safer.

Thanks - fixed. I left the delay as there was a bug reported about file write delay but then it joins back to main now

…l app - that requires more CI work

dialog/file.go

andydotxyz · 2025-01-10T22:02:19Z

PASS!!!

Jacalz

Thanks. This is massively impressive. Well done! I left some notes and questions in-line :)

Jacalz · 2025-01-10T22:57:47Z

app/cloud_test.go

 	"fyne.io/fyne/v2/theme"

 	"github.com/stretchr/testify/assert"
 )

 func TestFyneApp_SetCloudProvider(t *testing.T) {
-	a := NewWithID("io.fyne.test")
+	a := test.NewApp()


I wonder if we ought to use the new test.NewTempApp() in all these new cases?

Jacalz · 2025-01-10T23:00:12Z

app/preferences.go

@@ -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.


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?

In Go, calling .Write() on a file returns how many bytes it has written so it must be blocking until the write is complete. If this code and the write is happening on the same thread, we should not see any problem as far as I can tell.

As long as we are not reading and writing from different threads, I don't think atomic writes should matter much.

dialog/file.go

Jacalz · 2025-01-10T23:05:52Z

internal/driver/mobile/canvas_test.go

 func newTestMobileApp() fyne.App {
 	return &mobileApp{
 		App:    fyne.CurrentApp(),
 		driver: NewGoMobileDriver(),
 	}
 }
+
+func waitAndCheck(msWait time.Duration, fn func()) {
+	waitForCheck := make(chan struct{})


Should this not use the common pool of done channels?

Jacalz · 2025-01-10T23:08:53Z

test/app.go

+	s.app.propertyLock.Lock()
+	s.app.driver.CallFromGoroutine(func() {
+		painter.ClearFontCache()
+		cache.ResetThemeCaches()
+		intapp.ApplySettings(s, s.app)
+	})
+
+	s.app.appliedTheme = s.Theme()
+	s.app.propertyLock.Unlock()


This locking around it all seems strange to me. Is it really safe? Should it not be in the scheduled function?

Jacalz · 2025-01-10T23:09:43Z

widget/entry_cursor_anim.go

 	if isStopped {
 		a.anim.Start()
 	}


Should this be combined with the block above?

andydotxyz added 11 commits January 7, 2025 15:50

Synchronise the theme watcher and other "background" items in test code

8412c9d

Refactored test package to not have races in the app settings applica…

d35da3e

…tion

Resolve a race in the preference code

5ae7d16

Remove lock complications in test, they now behave better

d28e1c5

Correct race in mobile double tap handling

145dd07

Also actually run the mobile driver during tests

Remove locks and fix thread safety of cursor animation

89f5f20

Move the window safety code into a wrapper type

b0e5c64

Fixes fyne-io#5375

Fixes to canvas tests and what looked like 2 bad test files

08d798a

The final runOnMains for testing, and enable race checks in CI!

00d741f

Merge branch 'develop' into fix/racedetection

4c0458c

Fix merge conflict

7d5fb07

andydotxyz force-pushed the fix/racedetection branch from d1bbb2a to 7d5fb07 Compare January 9, 2025 17:40

andydotxyz added 2 commits January 9, 2025 17:50

Always use safe canvas in tests

367a6a6

Fix race in animation tests

895953c

Don't use goroutines in preference applciation

13d1f77

andydotxyz added 2 commits January 9, 2025 18:16

Add race to mobile and wayland

4713739

Fixing some more window test races

a0ecb94

andydotxyz added 2 commits January 9, 2025 20:23

Missed another goroutine

98dc17d

Remove test that doesn't agree with pool contract

b97878b

Jacalz mentioned this pull request Jan 9, 2025

Cleanup of unneeded locks in canvas objects #5388

Open

3 tasks

andydotxyz added 5 commits January 10, 2025 13:06

Correcting test harness to fully run mobile and fix threading of doub…

06b616b

…le tap mobile

Be consistent with GLFW and wait for the main request

d4f57b7

Attempting full mobile run

6d8675d

Merge branch 'develop' into fix/racedetection

ddacc2c

More safety in preference write

fb997d8

andydotxyz added 3 commits January 10, 2025 16:33

Lighter weight mobile test - more than before this PR but not the ful…

afb5db5

…l app - that requires more CI work

Potential issue with preferences and mobile

52a15d9

Adjusting thread on file watcher callbacks

a446af3

andydotxyz self-assigned this Jan 10, 2025

andydotxyz and others added 12 commits January 10, 2025 17:11

Fix staticcheck

154d1e6

More GLFW fixes

c2f8169

Correct threading of cloud apply test

2b9f46d

Don't "find" app files for dialog tests

b37b4a5

Fix a race in menu bar tests on Ubuntu

df498a6

Menu item safety for Ubuntu tests too

49cbf18

GLFW tests need real main thread

190d747

Remove last worrying goroutines

f04b0c9

Same fix avoiding import loop

6cec612

Safer tapping around in GLFW tests

1ae8045

Protecting more complex tap flows

bcb6742

Passing locally on Linux now too

15b454e

dweymouth reviewed Jan 10, 2025

View reviewed changes

dialog/file.go Show resolved Hide resolved

andydotxyz added 2 commits January 10, 2025 21:42

Fixing some intermittent

7dbcac0

Fixing another occasional race

62d9f9a

Jacalz requested changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

That's the races out of our test suite! #5384

That's the races out of our test suite! #5384

andydotxyz commented Jan 9, 2025

andydotxyz commented Jan 9, 2025

dweymouth commented Jan 9, 2025

andydotxyz commented Jan 9, 2025

andydotxyz commented Jan 9, 2025

dweymouth commented Jan 9, 2025

coveralls commented Jan 9, 2025

coveralls commented Jan 9, 2025

coveralls commented Jan 9, 2025

coveralls commented Jan 9, 2025 •

edited

Loading

dweymouth commented Jan 9, 2025

andydotxyz commented Jan 9, 2025

dweymouth commented Jan 9, 2025

Jacalz commented Jan 9, 2025

andydotxyz commented Jan 10, 2025

andydotxyz commented Jan 10, 2025

Jacalz left a comment

Jacalz Jan 10, 2025

Jacalz Jan 10, 2025

Jacalz Jan 10, 2025

Jacalz Jan 10, 2025

Jacalz Jan 10, 2025

Jacalz Jan 10, 2025

That's the races out of our test suite! #5384

Are you sure you want to change the base?

That's the races out of our test suite! #5384

Conversation

andydotxyz commented Jan 9, 2025

Checklist:

andydotxyz commented Jan 9, 2025

dweymouth commented Jan 9, 2025

andydotxyz commented Jan 9, 2025

andydotxyz commented Jan 9, 2025

dweymouth commented Jan 9, 2025

coveralls commented Jan 9, 2025

coveralls commented Jan 9, 2025

coveralls commented Jan 9, 2025

coveralls commented Jan 9, 2025 • edited Loading

dweymouth commented Jan 9, 2025

andydotxyz commented Jan 9, 2025

dweymouth commented Jan 9, 2025

Jacalz commented Jan 9, 2025

andydotxyz commented Jan 10, 2025

andydotxyz commented Jan 10, 2025

Jacalz left a comment

Choose a reason for hiding this comment

Jacalz Jan 10, 2025

Choose a reason for hiding this comment

Jacalz Jan 10, 2025

Choose a reason for hiding this comment

Jacalz Jan 10, 2025

Choose a reason for hiding this comment

Jacalz Jan 10, 2025

Choose a reason for hiding this comment

Jacalz Jan 10, 2025

Choose a reason for hiding this comment

Jacalz Jan 10, 2025

Choose a reason for hiding this comment

coveralls commented Jan 9, 2025 •

edited

Loading