Skip to content
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

internal/cache: use expiringCache over expiringCacheNoLock in svgInfo #3527

Merged
merged 1 commit into from
Jan 2, 2023

Conversation

changkun
Copy link
Member

@changkun changkun commented Dec 31, 2022

Description:

As of #2659, on darwin/arm64, most of the draw operations are run on the main thread. This mitigates many data races to appear in the official demo. However, after #2812, a new type of data race can appear at boot time:

$ cd cmd/fyne_demo && go build -race
$ ./fyne_demo
2022/12/31 12:26:47 Lifecycle: Started
==================
WARNING: DATA RACE
Write at 0x00c0004dc960 by goroutine 14:
  fyne.io/fyne/v2/internal/cache.(*expiringCacheNoLock).setAlive()
      /Users/changkun/dev/changkun.de/fyne/internal/cache/base.go:246 +0x94
  fyne.io/fyne/v2/internal/cache.GetSvg()
      /Users/changkun/dev/changkun.de/fyne/internal/cache/svg.go:22 +0xd4
  fyne.io/fyne/v2/internal/painter.paintImage()
      /Users/changkun/dev/changkun.de/fyne/internal/painter/image.go:107 +0x3e0
  fyne.io/fyne/v2/internal/painter.PaintImage()
      /Users/changkun/dev/changkun.de/fyne/internal/painter/image.go:54 +0xfc
  fyne.io/fyne/v2/internal/driver/glfw.itemForMenuItem()
      /Users/changkun/dev/changkun.de/fyne/internal/driver/glfw/driver_desktop.go:89 +0x340
  fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).refreshSystrayMenu()
      /Users/changkun/dev/changkun.de/fyne/internal/driver/glfw/driver_desktop.go:117 +0x94
  fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).refreshSystray()
      /Users/changkun/dev/changkun.de/fyne/internal/driver/glfw/driver_desktop.go:105 +0x80
  fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).SetSystemTrayMenu.func1.1()
      /Users/changkun/dev/changkun.de/fyne/internal/driver/glfw/driver_desktop.go:46 +0xe8
  fyne.io/systray.Register.func2()
      /Users/changkun/dev/changkun.de/fyne/vendor/fyne.io/systray/systray.go:102 +0x44

Previous read at 0x00c0004dc960 by main goroutine:
  fyne.io/fyne/v2/internal/cache.(*expiringCacheNoLock).isExpired()
      /Users/changkun/dev/changkun.de/fyne/internal/cache/base.go:240 +0xac
  fyne.io/fyne/v2/internal/cache.destroyExpiredSvgs.func1()
      /Users/changkun/dev/changkun.de/fyne/internal/cache/svg.go:48 +0xa0
  sync.(*Map).Range()
      /Users/changkun/goes/go/src/sync/map.go:354 +0x1b4
  fyne.io/fyne/v2/internal/cache.destroyExpiredSvgs()
      /Users/changkun/dev/changkun.de/fyne/internal/cache/svg.go:46 +0xb4
  fyne.io/fyne/v2/internal/cache.CleanCanvases()
      /Users/changkun/dev/changkun.de/fyne/internal/cache/base.go:109 +0x124
  fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).drawSingleFrame()
      /Users/changkun/dev/changkun.de/fyne/internal/driver/glfw/loop.go:105 +0x64
  fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).runGL()
      /Users/changkun/dev/changkun.de/fyne/internal/driver/glfw/loop.go:176 +0x624
  fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).Run()
      /Users/changkun/dev/changkun.de/fyne/internal/driver/glfw/driver.go:168 +0xb8
  fyne.io/fyne/v2/internal/driver/glfw.(*window).ShowAndRun()
      /Users/changkun/dev/changkun.de/fyne/internal/driver/glfw/window.go:230 +0x44
  main.main()
      /Users/changkun/dev/changkun.de/fyne/cmd/fyne_demo/main.go:67 +0x5d8

Goroutine 14 (running) created at:
  fyne.io/systray.Register()
      /Users/changkun/dev/changkun.de/fyne/vendor/fyne.io/systray/systray.go:100 +0xe0
  fyne.io/systray.RunWithExternalLoop()
      /Users/changkun/dev/changkun.de/fyne/vendor/fyne.io/systray/systray.go:81 +0x30
  fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).SetSystemTrayMenu.func1()
      /Users/changkun/dev/changkun.de/fyne/internal/driver/glfw/driver_desktop.go:36 +0xd8
  sync.(*Once).doSlow()
      /Users/changkun/goes/go/src/sync/once.go:74 +0xb0
  sync.(*Once).Do()
      /Users/changkun/goes/go/src/sync/once.go:65 +0x40
  fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).SetSystemTrayMenu()
      /Users/changkun/dev/changkun.de/fyne/internal/driver/glfw/driver_desktop.go:35 +0x58
  fyne.io/fyne/v2/app.(*fyneApp).SetSystemTrayMenu()
      /Users/changkun/dev/changkun.de/fyne/app/app_desktop_darwin.go:30 +0x68
  main.makeTray()
      /Users/changkun/dev/changkun.de/fyne/cmd/fyne_demo/main.go:186 +0x340
  main.main()
      /Users/changkun/dev/changkun.de/fyne/cmd/fyne_demo/main.go:26 +0x80
==================
2022/12/31 12:26:48 Lifecycle: Entered Foreground
2022/12/31 12:26:49 Lifecycle: Exited Foreground
2022/12/31 12:26:49 Lifecycle: Stopped
Found 1 data race(s)

This is because systray is running on a separate goroutine, and therefore the SVG caches are accessed from multiple goroutines.

Instead of using expiringCacheNoLock, switch to expiringCache to eliminate this type of data race.

Updates #2509

Checklist:

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

@coveralls
Copy link

Coverage Status

Coverage: 62.029% (+0.03%) from 62.002% when pulling 96c514e on changkun:expirelock into de4a964 on fyne-io:develop.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

The M1 not having the render/main race is probably temporary as we only reverted to 1 thread as a workaround until GLFW upstream finds the source of the problem.

@changkun changkun merged commit cffdaf5 into fyne-io:develop Jan 2, 2023
@changkun changkun deleted the expirelock branch January 2, 2023 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants