Refactoring of the pluggable discovery client

arduino/discovery/discovery.go

@@ -32,17 +32,6 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-// To work correctly a Pluggable Discovery must respect the state machine specified on the documentation:
-// https://arduino.github.io/arduino-cli/latest/pluggable-discovery-specification/#state-machine
-// States a PluggableDiscovery can be in
-const (
-	Alive int = iota
-	Idling
-	Running
-	Syncing
-	Dead
-)
-
 // PluggableDiscovery is a tool that detects communication ports to interact
 // with the boards.
 type PluggableDiscovery struct {
@@ -55,7 +44,6 @@ type PluggableDiscovery struct {
 	// All the following fields are guarded by statusMutex
 	statusMutex           sync.Mutex
 	incomingMessagesError error
-	state                 int
 	eventChan             chan<- *Event
 }
 
@@ -168,7 +156,6 @@ func New(id string, args ...string) *PluggableDiscovery {
 	return &PluggableDiscovery{
 		id:          id,
 		processArgs: args,
-		state:       Dead,
 	}
 }
 
@@ -185,28 +172,26 @@ func (disc *PluggableDiscovery) jsonDecodeLoop(in io.Reader, outChan chan<- *dis
 	decoder := json.NewDecoder(in)
 	closeAndReportError := func(err error) {
 		disc.statusMutex.Lock()
-		disc.state = Dead
 		disc.incomingMessagesError = err
 		disc.statusMutex.Unlock()
+		disc.killProcess()
 		close(outChan)
-		logrus.Errorf("stopped discovery %s decode loop: %v", disc.id, err)
+		if err != nil {
+			logrus.Errorf("stopped discovery %s decode loop: %v", disc, err)
+		}
 	}
 
 	for {
 		var msg discoveryMessage
 		if err := decoder.Decode(&msg); errors.Is(err, io.EOF) {
 			// This is fine, we exit gracefully
-			disc.statusMutex.Lock()
-			disc.state = Dead
-			disc.incomingMessagesError = err
-			disc.statusMutex.Unlock()
-			close(outChan)
+			closeAndReportError(nil)
 			return
 		} else if err != nil {
 			closeAndReportError(err)
 			return
 		}
-		logrus.Infof("from discovery %s received message %s", disc.id, msg)
+		logrus.Infof("from discovery %s received message %s", disc, msg)
 		if msg.EventType == "add" {
 			if msg.Port == nil {
 				closeAndReportError(errors.New(tr("invalid 'add' message: missing port")))
@@ -233,11 +218,10 @@ func (disc *PluggableDiscovery) jsonDecodeLoop(in io.Reader, outChan chan<- *dis
 	}
 }
 
-// State returns the current state of this PluggableDiscovery
-func (disc *PluggableDiscovery) State() int {
+func (disc *PluggableDiscovery) Alive() bool {
 	disc.statusMutex.Lock()
 	defer disc.statusMutex.Unlock()
-	return disc.state
+	return disc.process != nil
 }
 
 func (disc *PluggableDiscovery) waitMessage(timeout time.Duration) (*discoveryMessage, error) {
@@ -268,7 +252,7 @@ func (disc *PluggableDiscovery) sendCommand(command string) error {
 }
 
 func (disc *PluggableDiscovery) runProcess() error {
-	logrus.Infof("starting discovery %s process", disc.id)
+	logrus.Infof("starting discovery %s process", disc)
 	proc, err := executils.NewProcess(nil, disc.processArgs...)
 	if err != nil {
 		return err
@@ -294,26 +278,25 @@ func (disc *PluggableDiscovery) runProcess() error {
 	disc.statusMutex.Lock()
 	defer disc.statusMutex.Unlock()
 	disc.process = proc
-	disc.state = Alive
-	logrus.Infof("started discovery %s process", disc.id)
+	logrus.Infof("started discovery %s process", disc)
 	return nil
 }
 
 func (disc *PluggableDiscovery) killProcess() error {
-	logrus.Infof("killing discovery %s process", disc.id)
+	disc.statusMutex.Lock()
+	defer disc.statusMutex.Unlock()
+	logrus.Infof("killing discovery %s process", disc)
 	if disc.process != nil {
 		if err := disc.process.Kill(); err != nil {
 			return err
 		}
 		if err := disc.process.Wait(); err != nil {
 			return err
 		}
+		disc.process = nil
 	}
-	disc.statusMutex.Lock()
-	defer disc.statusMutex.Unlock()
 	disc.stopSync()
-	disc.state = Dead
-	logrus.Infof("killed discovery %s process", disc.id)
+	logrus.Infof("killed discovery %s process", disc)
 	return nil
 }
 
@@ -353,14 +336,11 @@ func (disc *PluggableDiscovery) Run() (err error) {
 	} else if msg.ProtocolVersion > 1 {
 		return errors.Errorf(tr("protocol version not supported: requested 1, got %d"), msg.ProtocolVersion)
 	}
-	disc.statusMutex.Lock()
-	defer disc.statusMutex.Unlock()
-	disc.state = Idling
 	return nil
 }
 
 // Start initializes and start the discovery internal subroutines. This command must be
-// called before List or StartSync.
+// called before List.
 func (disc *PluggableDiscovery) Start() error {
 	if err := disc.sendCommand("START\n"); err != nil {
 		return err
@@ -374,9 +354,6 @@ func (disc *PluggableDiscovery) Start() error {
 	} else if strings.ToUpper(msg.Message) != "OK" {
 		return errors.Errorf(tr("communication out of sync, expected '%[1]s', received '%[2]s'"), "OK", msg.Message)
 	}
-	disc.statusMutex.Lock()
-	defer disc.statusMutex.Unlock()
-	disc.state = Running
 	return nil
 }
 
@@ -399,7 +376,6 @@ func (disc *PluggableDiscovery) Stop() error {
 	disc.statusMutex.Lock()
 	defer disc.statusMutex.Unlock()
 	disc.stopSync()
-	disc.state = Idling
 	return nil
 }
 
@@ -415,7 +391,7 @@ func (disc *PluggableDiscovery) stopSync() {
 func (disc *PluggableDiscovery) Quit() {
 	_ = disc.sendCommand("QUIT\n")
 	if _, err := disc.waitMessage(time.Second * 5); err != nil {
-		logrus.Errorf("Quitting discovery %s: %s", disc.id, err)
+		logrus.Errorf("Quitting discovery %s: %s", disc, err)
 	}
 	disc.stopSync()
 	disc.killProcess()
@@ -463,7 +439,6 @@ func (disc *PluggableDiscovery) StartSync(size int) (<-chan *Event, error) {
 		return nil, errors.Errorf(tr("communication out of sync, expected '%[1]s', received '%[2]s'"), "OK", msg.Message)
 	}
 
-	disc.state = Syncing
 	// In case there is already an existing event channel in use we close it before creating a new one.
 	disc.stopSync()
 	c := make(chan *Event, size)

arduino/discovery/discovery_test.go

@@ -63,11 +63,11 @@ func TestDiscoveryStdioHandling(t *testing.T) {
 	require.NotNil(t, msg)
 	require.Equal(t, "ev2", msg.EventType)
 
-	require.Equal(t, disc.State(), Alive)
+	require.True(t, disc.Alive())
 
 	err = disc.outgoingCommandsPipe.(io.ReadCloser).Close()
 	require.NoError(t, err)
 	time.Sleep(time.Millisecond * 100)
 
-	require.Equal(t, disc.State(), Dead)
+	require.False(t, disc.Alive())
 }