Prevent potential fsnotify deadlock issue

Based on recommendations here: https://github.com/fsnotify/fsnotify#faq Issue #68
robgonnella · Jul 11, 2021 · eef0eae · eef0eae
1 parent 724dbf9
commit eef0eae
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 64 deletions.
diff --git a/v2/commands/attach_and_watch.go b/v2/commands/attach_and_watch.go
@@ -2,9 +2,6 @@ package commands
 
 import (
 	"errors"
-	"os"
-	"os/signal"
-	"syscall"
 
 	cli "github.com/robgonnella/ardi/v2/cli-wrapper"
 	"github.com/robgonnella/ardi/v2/core"
@@ -76,15 +73,7 @@ func getWatchCmd() *cobra.Command {
 			}
 			ardiCore.Watcher.SetTargets(targets)
 
-			sigs := make(chan os.Signal, 1)
-			signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
-
-			go func() {
-				<-sigs
-				logger.Debug("gracefully shutting down attach-and-watch")
-				ardiCore.Watcher.Stop()
-			}()
-
+			defer ardiCore.Watcher.Stop()
 			return ardiCore.Watcher.Watch()
 		},
 	}

diff --git a/v2/commands/compile.go b/v2/commands/compile.go
@@ -2,9 +2,6 @@ package commands
 
 import (
 	"errors"
-	"os"
-	"os/signal"
-	"syscall"
 
 	"github.com/robgonnella/ardi/v2/core"
 	"github.com/spf13/cobra"
@@ -26,6 +23,8 @@ func getCompileCmd() *cobra.Command {
 		Short:   "Compile specified sketch or build(s)",
 		Aliases: []string{"build"},
 		RunE: func(cmd *cobra.Command, args []string) error {
+			defer ardiCore.Compiler.StopWatching() // noop if not watching
+
 			ardiBuilds := ardiCore.Config.GetBuilds()
 
 			if all {
@@ -51,17 +50,11 @@ func getCompileCmd() *cobra.Command {
 				if err != nil {
 					return err
 				}
-				if watch {
-					sigs := make(chan os.Signal, 1)
-					signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
 
-					go func() {
-						<-sigs
-						logger.Debug("gracefully shutting down file watcher")
-						ardiCore.Compiler.StopWatching()
-					}()
+				if watch {
 					return ardiCore.Compiler.WatchForChanges(*opts)
 				}
+
 				return nil
 			}
 

diff --git a/v2/commands/upload.go b/v2/commands/upload.go
@@ -2,9 +2,6 @@ package commands
 
 import (
 	"errors"
-	"os"
-	"os/signal"
-	"syscall"
 
 	cli "github.com/robgonnella/ardi/v2/cli-wrapper"
 	"github.com/robgonnella/ardi/v2/types"
@@ -24,6 +21,8 @@ func getUploadCmd() *cobra.Command {
 			"the sketch argument matches a user defined build in ardi.json, the " +
 			"build values will be used to find the appropraite build to upload",
 		RunE: func(cmd *cobra.Command, args []string) error {
+			defer ardiCore.Uploader.Detach() // noop if not attached
+
 			builds := ardiCore.Config.GetBuilds()
 
 			build := "."
@@ -52,16 +51,16 @@ func getUploadCmd() *cobra.Command {
 				}
 			}
 
+			if board == nil || board.FQBN == "" || board.Port == "" {
+				return errors.New("no connected boards detected")
+			}
+
 			fields := logrus.Fields{
 				"build":  project.Directory,
 				"fqbn":   board.FQBN,
 				"device": board.Port,
 			}
-
 			logger.WithFields(fields).Info("Uploading...")
-			if board == nil || board.FQBN == "" || board.Port == "" {
-				return errors.New("no connected boards detected")
-			}
 
 			if err := ardiCore.Uploader.Upload(board, project.Directory); err != nil {
 				return err
@@ -70,15 +69,6 @@ func getUploadCmd() *cobra.Command {
 			logger.Info("Upload successful")
 
 			if attach {
-				sigs := make(chan os.Signal, 1)
-				signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
-
-				go func() {
-					<-sigs
-					logger.Debug("gracefully shutting down serail port logger")
-					ardiCore.Uploader.Detach()
-				}()
-
 				ardiCore.Uploader.Attach(board.Port, project.Baud, nil)
 			}
 

diff --git a/v2/core/file_watcher.go b/v2/core/file_watcher.go
@@ -57,12 +57,43 @@ func (f *FileWatcher) Watch() error {
 
 	f.logger.Infof("Watching %s for changes", f.file)
 
-	for {
-		if f.watcher == nil {
-			f.logger.Debug("file watcher already closed")
-			return nil
+	// Run select statement for fsnotify events in separate goroutine
+	// as recommended here : https://github.com/fsnotify/fsnotify#faq
+	go func() {
+		for {
+			if f.watcher == nil {
+				f.logger.Debug("file watcher already closed")
+				return
+			}
+
+			select {
+			case event, ok := <-f.watcher.Events:
+				if !ok {
+					f.logger.Error("unknown file watch error")
+					return
+				}
+				f.logger.Debugf("event: %+v", event)
+				if event.Op&fsnotify.Write == fsnotify.Write {
+					f.logger.Debugf("modified file: %s", event.Name)
+					for _, l := range f.listeners {
+						go l()
+					}
+				}
+			case err, ok := <-f.watcher.Errors:
+				if !ok {
+					f.logger.Debug("unknown file watch error")
+					return
+				}
+				f.watcher.Close()
+				f.watcher = nil
+				f.logger.WithError(err).Error("Watch error")
+				return
+			}
 		}
+	}()
 
+	// Block and wait for requests
+	for {
 		select {
 		case <-f.stop:
 			f.watcher.Remove(f.file)
@@ -75,27 +106,6 @@ func (f *FileWatcher) Watch() error {
 			f.watcher.Close()
 			f.watcher = nil
 			return nil
-		case event, ok := <-f.watcher.Events:
-			if !ok {
-				f.logger.Error("unknown file watch error")
-				return nil
-			}
-			f.logger.Debugf("event: %+v", event)
-			if event.Op&fsnotify.Write == fsnotify.Write {
-				f.logger.Debugf("modified file: %s", event.Name)
-				for _, l := range f.listeners {
-					go l()
-				}
-			}
-		case err, ok := <-f.watcher.Errors:
-			if !ok {
-				f.logger.Debug("unknown file watch error")
-				return nil
-			}
-			f.watcher.Close()
-			f.watcher = nil
-			f.logger.WithError(err).Error("Watch error")
-			return err
 		}
 	}
 }