core icon indicating copy to clipboard operation
core copied to clipboard

Data race when attempting to close main window from another goroutine

Open runrc opened this issue 1 year ago • 1 comments

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/[email protected]/core/renderwindow.go:588 +0xd0
  cogentcore.org/core/core.(*WidgetBase).AsyncLock()
      /Users/test/go/pkg/mod/cogentcore.org/[email protected]/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/[email protected]/core/renderwindow.go:570 +0x34
  cogentcore.org/core/core.(*Stage).newRenderWindow()
      /Users/test/go/pkg/mod/cogentcore.org/[email protected]/core/mainstage.go:380 +0x458
  cogentcore.org/core/core.(*Stage).runWindow()
      /Users/test/go/pkg/mod/cogentcore.org/[email protected]/core/mainstage.go:255 +0x7ec
  cogentcore.org/core/core.(*Stage).run()
      /Users/test/go/pkg/mod/cogentcore.org/[email protected]/core/stage.go:293 +0xb4
  cogentcore.org/core/core.(*Stage).Run()
      /Users/test/go/pkg/mod/cogentcore.org/[email protected]/core/stage.go:272 +0xc8
  cogentcore.org/core/core.(*Body).RunWindow()
      /Users/test/go/pkg/mod/cogentcore.org/[email protected]/core/mainstage.go:71 +0x98
  cogentcore.org/core/core.(*Body).RunMainWindow()
      /Users/test/go/pkg/mod/cogentcore.org/[email protected]/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

runrc avatar Oct 21 '24 17:10 runrc

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 avatar Oct 21 '24 19:10 kkoreilly

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.

kkoreilly avatar Nov 08 '24 03:11 kkoreilly

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()

runrc avatar Nov 09 '24 15:11 runrc

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.

kkoreilly avatar Nov 14 '24 22:11 kkoreilly

Yes this works perfectly. Thank you.

runrc avatar Nov 17 '24 14:11 runrc