Merge pull request #37 from thiagokokada/fixing-data-races

Fixing data races

.github/workflows/go.yml

@@ -31,7 +31,7 @@ jobs:
         go run honnef.co/go/tools/cmd/[email protected] ./...
 
     - name: Test
-      run: go test -v ./...
+      run: go test -race -v ./...
       env:
         CI: 1
 

core/core.go

@@ -89,9 +89,6 @@ func Start(
 	settings *Settings,
 	optional Optional,
 ) {
-	mu.Lock()
-	defer mu.Unlock()
-
 	if optional.Sound {
 		log.Printf(
 			"Running twenty-twenty-twenty every %.1f minute(s), with %.f second(s) duration and sound set to %t...\n",
@@ -107,11 +104,16 @@ func Start(
 		)
 	}
 	Stop() // make sure we cancel the previous instance
+
+	mu.Lock()
+	defer mu.Unlock()
 	loopCtx, cancelLoopCtx = context.WithCancel(context.Background())
 	go loop(settings)
 }
 
 func Stop() {
+	mu.Lock()
+	defer mu.Unlock()
 	if loopCtx != nil {
 		cancelLoopCtx()
 	}

core/core_test.go

@@ -3,37 +3,39 @@ package core
 import (
 	"context"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
 	"gioui.org/x/notify"
 	"github.com/thiagokokada/twenty-twenty-twenty/notification"
+	"golang.org/x/exp/constraints"
 )
 
 type mockNotifier struct {
 	notify.Notifier
-	notificationCancelCount *int
-	notificationCount       *int
+	notificationCancelCount atomic.Int32
+	notificationCount       atomic.Int32
 }
 
 type mockNotification struct {
 	*mockNotifier
 }
 
-func (n mockNotifier) CreateNotification(title, text string) (notify.Notification, error) {
-	*n.notificationCount++
-	return &mockNotification{mockNotifier: &n}, nil
+func (n *mockNotifier) CreateNotification(title, text string) (notify.Notification, error) {
+	n.notificationCount.Add(1)
+	return &mockNotification{mockNotifier: n}, nil
 }
 
-func (n mockNotification) Cancel() error {
-	*n.mockNotifier.notificationCancelCount++
+func (n *mockNotification) Cancel() error {
+	n.notificationCancelCount.Add(1)
 	return nil
 }
 
-func newMockNotifier() mockNotifier {
-	return mockNotifier{
-		notificationCancelCount: new(int),
-		notificationCount:       new(int),
+func newMockNotifier() *mockNotifier {
+	return &mockNotifier{
+		notificationCancelCount: atomic.Int32{},
+		notificationCount:       atomic.Int32{},
 	}
 }
 
@@ -44,7 +46,7 @@ func assertEqual[T comparable](t *testing.T, actual, expected T) {
 	}
 }
 
-func assertGreaterOrEqual(t *testing.T, actual, expected int) {
+func assertGreaterOrEqual[T constraints.Ordered](t *testing.T, actual, expected T) {
 	t.Helper()
 	if actual < expected {
 		t.Errorf("got: %v; want: >=%v", actual, expected)
@@ -90,14 +92,14 @@ func TestStart(t *testing.T) {
 
 	const timeout = time.Second
 	// the last notification may or may not come because of timing
-	expectCount := int(timeout/testSettings.Frequency) - 1
+	expectCount := int32(timeout/testSettings.Frequency) - 1
 
 	Start(&testSettings, Optional{Sound: true})
 	defer Stop()
 	time.Sleep(timeout)
 
-	assertGreaterOrEqual(t, *notifier.notificationCount, expectCount)
-	assertGreaterOrEqual(t, *notifier.notificationCancelCount, expectCount)
+	assertGreaterOrEqual(t, notifier.notificationCount.Load(), expectCount)
+	assertGreaterOrEqual(t, notifier.notificationCancelCount.Load(), expectCount)
 }
 
 func TestPause(t *testing.T) {
@@ -119,8 +121,8 @@ func TestPause(t *testing.T) {
 
 	assertEqual(t, callbackPreCalled, true)
 	assertEqual(t, callbackPosCalled, true)
-	assertGreaterOrEqual(t, *notifier.notificationCount, 1)
-	assertGreaterOrEqual(t, *notifier.notificationCancelCount, 1)
+	assertGreaterOrEqual(t, notifier.notificationCount.Load(), 1)
+	assertGreaterOrEqual(t, notifier.notificationCancelCount.Load(), 1)
 }
 
 func TestPauseCancel(t *testing.T) {
@@ -143,6 +145,6 @@ func TestPauseCancel(t *testing.T) {
 
 	assertEqual(t, callbackPreCalled, false)
 	assertEqual(t, callbackPosCalled, false)
-	assertEqual(t, *notifier.notificationCount, 0)
-	assertEqual(t, *notifier.notificationCancelCount, 0)
+	assertEqual(t, notifier.notificationCount.Load(), 0)
+	assertEqual(t, notifier.notificationCancelCount.Load(), 0)
 }

go.mod

@@ -9,6 +9,7 @@ require (
 	gioui.org/cmd v0.0.0-20240115171100-84ca391d514b
 	gioui.org/x v0.4.0
 	github.com/gopxl/beep v1.3.0
+	golang.org/x/exp v0.0.0-20240112132812-db7319d0e0e3
 )
 
 require (
@@ -28,7 +29,6 @@ require (
 	github.com/jfreymuth/vorbis v1.0.2 // indirect
 	github.com/pkg/errors v0.9.1 // indirect
 	github.com/tevino/abool v1.2.0 // indirect
-	golang.org/x/exp v0.0.0-20240112132812-db7319d0e0e3 // indirect
 	golang.org/x/exp/shiny v0.0.0-20240112132812-db7319d0e0e3 // indirect
 	golang.org/x/image v0.15.0 // indirect
 	golang.org/x/mod v0.14.0 // indirect

notification/notification.go

@@ -4,14 +4,15 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"sync/atomic"
 	"time"
 
 	"gioui.org/x/notify"
 
 	snd "github.com/thiagokokada/twenty-twenty-twenty/sound"
 )
 
-var notifier notify.Notifier
+var notifier atomic.Pointer[notify.Notifier]
 
 func SendWithDuration(
 	ctx context.Context,
@@ -39,7 +40,7 @@ func Send(
 	if *sound {
 		snd.PlaySendNotification(func() {})
 	}
-	return notifier.CreateNotification(title, text)
+	return (*notifier.Load()).CreateNotification(title, text)
 }
 
 func CancelAfter(
@@ -64,11 +65,11 @@ func CancelAfter(
 }
 
 func SetNotifier(n notify.Notifier) {
-	notifier = n
+	notifier.Store(&n)
 }
 
 func initIfNull() {
-	if notifier == nil {
+	if notifier.Load() == nil {
 		notifier, err := notify.NewNotifier()
 		if err != nil {
 			log.Fatalf("Error while creating a notifier: %v\n", err)