Fixed race issue (#30)

* Fixed race issues * Updated .gitignore * Added race detection in CI tests * Fixed lint issues * Fixed issue in tests
evg4b · Apr 21, 2024 · 0befa1e · 0befa1e
1 parent 18f2f19
commit 0befa1e
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 26 deletions.
diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
@@ -32,7 +32,7 @@ jobs:
         run: go build -tags release -v .
 
       - name: Test
-        run: go test -tags release -timeout 1m -v -coverprofile=coverage.out ./...
+        run: go test -tags release -timeout 1m -race -v -coverprofile=coverage.out ./...
 
       - name: SonarCloud Scan
         uses: SonarSource/sonarcloud-github-action@master

diff --git a/.gitignore b/.gitignore
@@ -5,3 +5,4 @@ dist/
 .idea
 node_modules
 .uncors.yaml
+coverage.out
diff --git a/internal/helpers/graceful_shutdown_internal_test.go b/internal/helpers/graceful_shutdown_internal_test.go
@@ -5,6 +5,7 @@ import (
 	"os"
 	"os/signal"
 	"sync"
+	"sync/atomic"
 	"syscall"
 	"testing"
 	"time"
@@ -16,6 +17,7 @@ import (
 type Env struct {
 	wg       *sync.WaitGroup
 	afterAll []func()
+	mutex    sync.Mutex
 }
 
 func (e *Env) Go(action func()) {
@@ -30,10 +32,18 @@ func (e *Env) CheckAfterAll(action func()) {
 	e.afterAll = append(e.afterAll, action)
 }
 
-func WithGoroutines(test func(t *testing.T, env Env)) func(t *testing.T) {
+func (e *Env) Lock() {
+	e.mutex.Lock()
+}
+
+func (e *Env) Unlock() {
+	e.mutex.Unlock()
+}
+
+func WithGoroutines(test func(t *testing.T, env *Env)) func(t *testing.T) {
 	return func(t *testing.T) {
-		env := Env{wg: &sync.WaitGroup{}}
-		test(t, env)
+		env := Env{wg: &sync.WaitGroup{}, mutex: sync.Mutex{}}
+		test(t, &env)
 		env.wg.Wait()
 		for _, f := range env.afterAll {
 			f()
@@ -42,21 +52,22 @@ func WithGoroutines(test func(t *testing.T, env Env)) func(t *testing.T) {
 }
 
 func TestGracefulShutdown(t *testing.T) {
-	t.Run("shutdown when context is done", WithGoroutines(func(t *testing.T, env Env) {
+	t.Run("shutdown when context is done", WithGoroutines(func(t *testing.T, env *Env) {
 		ctx, cancel := context.WithCancel(context.Background())
 
-		called := false
+		called := &atomic.Bool{}
+
 		env.Go(func() {
 			err := GracefulShutdown(ctx, func(_ context.Context) error {
-				called = true
+				called.Store(true)
 
 				return nil
 			})
 			require.NoError(t, err)
 		})
 
 		env.CheckAfterAll(func() {
-			assert.True(t, called)
+			assert.False(t, called.Load())
 		})
 
 		cancel()
@@ -82,10 +93,12 @@ func TestGracefulShutdown(t *testing.T) {
 		}
 
 		for _, testCase := range tests {
-			t.Run(testCase.name, WithGoroutines(func(t *testing.T, env Env) {
+			t.Run(testCase.name, WithGoroutines(func(t *testing.T, env *Env) {
+				env.Lock()
 				var systemSig chan<- os.Signal
 				notifyFn = func(c chan<- os.Signal, _ ...os.Signal) {
 					systemSig = c
+					env.Unlock()
 				}
 
 				t.Cleanup(func() {
@@ -103,6 +116,7 @@ func TestGracefulShutdown(t *testing.T) {
 				})
 
 				<-time.After(50 * time.Millisecond)
+				env.Lock()
 				systemSig <- testCase.signal
 
 				env.CheckAfterAll(func() {
@@ -112,10 +126,12 @@ func TestGracefulShutdown(t *testing.T) {
 		}
 	})
 
-	t.Run("apply additional ui fix for SIGINT signal", WithGoroutines(func(t *testing.T, env Env) {
+	t.Run("apply additional ui fix for SIGINT signal", WithGoroutines(func(t *testing.T, env *Env) {
 		var systemSig chan<- os.Signal
+		env.Lock()
 		notifyFn = func(c chan<- os.Signal, _ ...os.Signal) {
 			systemSig = c
+			env.Unlock()
 		}
 		called := false
 		sigintFix = func() {
@@ -134,10 +150,12 @@ func TestGracefulShutdown(t *testing.T) {
 		})
 
 		<-time.After(50 * time.Millisecond)
+		env.Lock()
 		systemSig <- syscall.SIGINT
 
 		env.CheckAfterAll(func() {
 			assert.True(t, called)
 		})
+		env.Unlock()
 	}))
 }
diff --git a/internal/uncors/app.go b/internal/uncors/app.go
@@ -19,16 +19,18 @@ import (
 )
 
 type App struct {
-	fs            afero.Fs
-	version       string
-	waitGroup     *sync.WaitGroup
-	httpMutex     *sync.Mutex
-	httpsMutex    *sync.Mutex
-	server        *http.Server
-	shuttingDown  *atomic.Bool
-	httpListener  net.Listener
-	httpsListener net.Listener
-	cache         appCache
+	fs                 afero.Fs
+	version            string
+	waitGroup          *sync.WaitGroup
+	httpMutex          *sync.Mutex
+	httpsMutex         *sync.Mutex
+	server             *http.Server
+	shuttingDown       *atomic.Bool
+	httpListenerMutex  *sync.Mutex
+	httpListener       net.Listener
+	httpsListenerMutex *sync.Mutex
+	httpsListener      net.Listener
+	cache              appCache
 }
 
 const (
@@ -39,12 +41,14 @@ const (
 
 func CreateApp(fs afero.Fs, version string) *App {
 	return &App{
-		fs:           fs,
-		version:      version,
-		waitGroup:    &sync.WaitGroup{},
-		httpMutex:    &sync.Mutex{},
-		httpsMutex:   &sync.Mutex{},
-		shuttingDown: &atomic.Bool{},
+		fs:                 fs,
+		version:            version,
+		waitGroup:          &sync.WaitGroup{},
+		httpMutex:          &sync.Mutex{},
+		httpsMutex:         &sync.Mutex{},
+		shuttingDown:       &atomic.Bool{},
+		httpListenerMutex:  &sync.Mutex{},
+		httpsListenerMutex: &sync.Mutex{},
 	}
 }
 
@@ -139,10 +143,16 @@ func (app *App) Shutdown(ctx context.Context) error {
 }
 
 func (app *App) HTTPAddr() net.Addr {
+	app.httpListenerMutex.Lock()
+	defer app.httpListenerMutex.Unlock()
+
 	return app.httpListener.Addr() // TODO: Add nil handing
 }
 
 func (app *App) HTTPSAddr() net.Addr {
+	app.httpsListenerMutex.Lock()
+	defer app.httpsListenerMutex.Unlock()
+
 	return app.httpsListener.Addr() // TODO: Add nil handing
 }
 

diff --git a/internal/uncors/listen.go b/internal/uncors/listen.go
@@ -18,6 +18,8 @@ func (app *App) listenAndServe(addr string) error {
 		addr:  addr,
 		serve: app.server.Serve,
 		setListener: func(l net.Listener) {
+			app.httpListenerMutex.Lock()
+			defer app.httpListenerMutex.Unlock()
 			app.httpListener = l
 		},
 	})
@@ -30,6 +32,8 @@ func (app *App) listenAndServeTLS(addr string, certFile, keyFile string) error {
 			return app.server.ServeTLS(l, certFile, keyFile)
 		},
 		setListener: func(l net.Listener) {
+			app.httpsListenerMutex.Lock()
+			defer app.httpsListenerMutex.Unlock()
 			app.httpsListener = l
 		},
 	})