cogentcore / core

A free and open source framework for building powerful, fast, elegant 2D and 3D apps that run on macOS, Windows, Linux, iOS, Android, and the web with a single Go codebase, allowing you to Code Once, Run Everywhere.
http://cogentcore.org/core
BSD 3-Clause "New" or "Revised" License
1.75k stars 83 forks source link

Data race when attempting to close main window from another goroutine #1267

Closed runrc closed 1 week ago

runrc commented 1 month ago

Describe the bug

When attempting to close the main window using system.TheApp.Quit() from another goroutine (say a signal handler) causes a data race condition even when surrounded by AsyncLock() / AsyncUnlock()

How to reproduce

Call system.TheApp.Quit() from another goroutine.

Example code

sigCh := make(chan os.Signal, 1)
signal.Notify(sigCh, syscall.SIGHUP, syscall.SIGINT, syscall.SIGTERM, syscall.SIGQUIT)

b := c.NewBody("Test")

go func() {
   <-sigCh
   // Caught user interrupt, shutting down.

   b.AsyncLock()
   system.TheApp.Quit() // <--- causes a data race
   b.AsyncUnlock()
}()

c.NewButton(b).SetText("Hello, World!")
b.RunMainWindow()

Relevant output

==================
WARNING: DATA RACE
Write at 0x00c00038a2e8 by goroutine 34:
  ??()
      -:0 +0x10593a214
  sync/atomic.CompareAndSwapInt32()
      <autogenerated>:1 +0x18
  cogentcore.org/core/core.(*renderContext).lock()
      /Users/test/go/pkg/mod/cogentcore.org/core@v0.3.5/core/renderwindow.go:588 +0xd0
  cogentcore.org/core/core.(*WidgetBase).AsyncLock()
      /Users/test/go/pkg/mod/cogentcore.org/core@v0.3.5/core/render.go:35 +0xcc
  main.main.func1()
      /Users/test/Dev/test/test.go:29 +0xb8

Previous write at 0x00c00038a2e8 by main goroutine:
  cogentcore.org/core/core.newRenderContext()
      /Users/test/go/pkg/mod/cogentcore.org/core@v0.3.5/core/renderwindow.go:570 +0x34
  cogentcore.org/core/core.(*Stage).newRenderWindow()
      /Users/test/go/pkg/mod/cogentcore.org/core@v0.3.5/core/mainstage.go:380 +0x458
  cogentcore.org/core/core.(*Stage).runWindow()
      /Users/test/go/pkg/mod/cogentcore.org/core@v0.3.5/core/mainstage.go:255 +0x7ec
  cogentcore.org/core/core.(*Stage).run()
      /Users/test/go/pkg/mod/cogentcore.org/core@v0.3.5/core/stage.go:293 +0xb4
  cogentcore.org/core/core.(*Stage).Run()
      /Users/test/go/pkg/mod/cogentcore.org/core@v0.3.5/core/stage.go:272 +0xc8
  cogentcore.org/core/core.(*Body).RunWindow()
      /Users/test/go/pkg/mod/cogentcore.org/core@v0.3.5/core/mainstage.go:71 +0x98
  cogentcore.org/core/core.(*Body).RunMainWindow()
      /Users/test/go/pkg/mod/cogentcore.org/core@v0.3.5/core/mainstage.go:45 +0x58
  main.main()
      /Users/test/Dev/test/test.go:53 +0x644

Goroutine 34 (running) created at:
  main.main()
      /Users/test/Dev/test/test.go:25 +0x1c4
==================

Platform

macOS

kkoreilly commented 1 month ago

Thank you for reporting this. I am somewhat busy right now, but I will still be able to fix this soon (ideally within a few days, but definitely within a couple of weeks).

kkoreilly commented 2 weeks ago

It appears that the cause of the race condition is the theoretical possibility that you could get one of those signals as the app is starting up, at which point AsyncLock is not possible. Therefore, you can get rid of the race condition by putting your goroutine in OnShow:

    b.OnShow(func(e events.Event) {
        go func() {
            <-sigCh
            // Caught user interrupt, shutting down.

            b.AsyncLock()
            core.TheApp.Quit()
            b.AsyncUnlock()
        }()
    })

Please let me know if this fixes the issue.

runrc commented 1 week ago

Unfortunately, using the above code causes core.TheApp.Quit() to hang in renderwindow.go, func newRenderWindow(), at line 120 whilst attempting to acquire rc.lock()

kkoreilly commented 1 week ago

I fixed the underlying issue in #1309, so this should work now without any race conditions or hanging. You actually do not need to (and cannot) do AsyncLock here, since Quit already has the appropriate mutex protections. As such, this is the appropriate code for the relevant section:

    b.OnShow(func(e events.Event) {
        go func() {
            <-sigCh
            // Caught user interrupt, shutting down.

            core.TheApp.Quit()
        }()
    })

Please let me know if that works for you.

runrc commented 5 days ago

Yes this works perfectly. Thank you.