Fix gRPC BoardListWatch crashing when installing/uninstalling a core/lib (#1460)

Author: silvanocerza

arduino/discovery/discovery.go

@@ -168,13 +168,19 @@ func (disc *PluggableDiscovery) jsonDecodeLoop(in io.Reader, outChan chan<- *dis
 		disc.incomingMessagesError = err
 		disc.statusMutex.Unlock()
 		close(outChan)
-		logrus.Errorf("stopped discovery %s decode loop", disc.id)
-		// TODO: Try restarting process some times before closing it completely
+		logrus.Errorf("stopped discovery %s decode loop: %v", disc.id, err)
 	}
 
 	for {
 		var msg discoveryMessage
-		if err := decoder.Decode(&msg); err != nil {
+		if err := decoder.Decode(&msg); err == io.EOF {
+			// This is fine, we exit gracefully
+			disc.statusMutex.Lock()
+			disc.state = Dead
+			disc.statusMutex.Unlock()
+			close(outChan)
+			return
+		} else if err != nil {
 			closeAndReportError(err)
 			return
 		}
@@ -218,9 +224,6 @@ func (disc *PluggableDiscovery) waitMessage(timeout time.Duration) (*discoveryMe
 	select {
 	case msg := <-disc.incomingMessagesChan:
 		if msg == nil {
-			// channel has been closed
-			disc.statusMutex.Lock()
-			defer disc.statusMutex.Unlock()
 			return nil, disc.incomingMessagesError
 		}
 		return msg, nil

arduino/discovery/discoverymanager/discoverymanager.go

@@ -192,6 +192,9 @@ func (dm *DiscoveryManager) QuitAll() []error {
 	// Close the global channel only if there were no errors
 	// quitting all alive discoveries
 	if len(errs) == 0 && dm.globalEventCh != nil {
+		// Let events consumers that discoveries are quitting no more events
+		// will be sent on this channel
+		dm.globalEventCh <- &discovery.Event{Type: "quit"}
 		close(dm.globalEventCh)
 		dm.globalEventCh = nil
 	}

commands/board/list.go

@@ -258,6 +258,25 @@ func Watch(instanceID int32, interrupt <-chan bool) (<-chan *rpc.BoardListWatchR
 		for {
 			select {
 			case event := <-eventsChan:
+				if event.Type == "quit" {
+					// The discovery manager has closed its event channel because it's
+					// quitting all the discovery processes that are running, this
+					// means that the events channel we're listening from won't receive any
+					// more events.
+					// Handling this case is necessary when the board watcher is running and
+					// the instance being used is reinitialized since that quits all the
+					// discovery processes and reset the discovery manager. That would leave
+					// this goroutine listening forever on a "dead" channel and might even
+					// cause panics.
+					// This message avoid all this issues.
+					// It will be the client's task restarting the board watcher if necessary,
+					// this host won't attempt restarting it.
+					outChan <- &rpc.BoardListWatchResponse{
+						EventType: event.Type,
+					}
+					return
+				}
+
 				port := &rpc.DetectedPort{
 					Port: event.Port.ToRPC(),
 				}
@@ -276,11 +295,11 @@ func Watch(instanceID int32, interrupt <-chan bool) (<-chan *rpc.BoardListWatchR
 					Error:     boardsError,
 				}
 			case <-interrupt:
-				err := pm.DiscoveryManager().StopAll()
-				if err != nil {
+				errs := dm.StopAll()
+				if len(errs) > 0 {
 					outChan <- &rpc.BoardListWatchResponse{
 						EventType: "error",
-						Error:     tr("stopping discoveries: %s", err),
+						Error:     tr("stopping discoveries: %s", errs),
 					}
 					// Don't close the channel if quitting all discoveries
 					// failed, otherwise some processes might be left running.

commands/daemon/daemon.go

@@ -31,6 +31,8 @@ import (
 	"github.com/arduino/arduino-cli/i18n"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
 	"github.com/sirupsen/logrus"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 // ArduinoCoreServerImpl FIXMEDOC
@@ -109,6 +111,9 @@ func (s *ArduinoCoreServerImpl) BoardListWatch(stream rpc.ArduinoCoreService_Boa
 				logrus.Info("boards watcher stream closed")
 				interrupt <- true
 				return
+			} else if st, ok := status.FromError(err); ok && st.Code() == codes.Canceled {
+				logrus.Info("boards watcher interrupted by host")
+				return
 			} else if err != nil {
 				logrus.Infof("interrupting boards watcher: %v", err)
 				interrupt <- true

i18n/data/en.po

@@ -2388,12 +2388,12 @@ msgstr "board not found"
 msgid "boardname"
 msgstr "boardname"
 
-#: arduino/discovery/discovery.go:297
-#: arduino/discovery/discovery.go:318
-#: arduino/discovery/discovery.go:338
-#: arduino/discovery/discovery.go:361
-#: arduino/discovery/discovery.go:384
-#: arduino/discovery/discovery.go:407
+#: arduino/discovery/discovery.go:300
+#: arduino/discovery/discovery.go:321
+#: arduino/discovery/discovery.go:341
+#: arduino/discovery/discovery.go:364
+#: arduino/discovery/discovery.go:387
+#: arduino/discovery/discovery.go:410
 #: arduino/monitor/monitor.go:246
 msgid "calling %[1]s: %[2]w"
 msgstr "calling %[1]s: %[2]w"
@@ -2441,12 +2441,12 @@ msgstr "cleaning build path"
 msgid "command"
 msgstr "command"
 
-#: arduino/discovery/discovery.go:301
-#: arduino/discovery/discovery.go:322
-#: arduino/discovery/discovery.go:342
-#: arduino/discovery/discovery.go:365
-#: arduino/discovery/discovery.go:388
-#: arduino/discovery/discovery.go:411
+#: arduino/discovery/discovery.go:304
+#: arduino/discovery/discovery.go:325
+#: arduino/discovery/discovery.go:345
+#: arduino/discovery/discovery.go:368
+#: arduino/discovery/discovery.go:391
+#: arduino/discovery/discovery.go:414
 #: arduino/monitor/monitor.go:250
 #: arduino/monitor/monitor.go:270
 #: arduino/monitor/monitor.go:287
@@ -2467,32 +2467,32 @@ msgstr "communication out of sync, expected 'configure', received '%s'"
 msgid "communication out of sync, expected 'describe', received '%s'"
 msgstr "communication out of sync, expected 'describe', received '%s'"
 
-#: arduino/discovery/discovery.go:299
+#: arduino/discovery/discovery.go:302
 #: arduino/monitor/monitor.go:248
 msgid "communication out of sync, expected 'hello', received '%s'"
 msgstr "communication out of sync, expected 'hello', received '%s'"
 
-#: arduino/discovery/discovery.go:386
+#: arduino/discovery/discovery.go:389
 msgid "communication out of sync, expected 'list', received '%s'"
 msgstr "communication out of sync, expected 'list', received '%s'"
 
 #: arduino/monitor/monitor.go:321
 msgid "communication out of sync, expected 'open', received '%s'"
 msgstr "communication out of sync, expected 'open', received '%s'"
 
-#: arduino/discovery/discovery.go:363
+#: arduino/discovery/discovery.go:366
 msgid "communication out of sync, expected 'quit', received '%s'"
 msgstr "communication out of sync, expected 'quit', received '%s'"
 
-#: arduino/discovery/discovery.go:320
+#: arduino/discovery/discovery.go:323
 msgid "communication out of sync, expected 'start', received '%s'"
 msgstr "communication out of sync, expected 'start', received '%s'"
 
-#: arduino/discovery/discovery.go:409
+#: arduino/discovery/discovery.go:412
 msgid "communication out of sync, expected 'start_sync', received '%s'"
 msgstr "communication out of sync, expected 'start_sync', received '%s'"
 
-#: arduino/discovery/discovery.go:340
+#: arduino/discovery/discovery.go:343
 msgid "communication out of sync, expected 'stop', received '%s'"
 msgstr "communication out of sync, expected 'stop', received '%s'"
 
@@ -2706,11 +2706,11 @@ msgstr "installing %[1]s tool: %[2]s"
 msgid "installing platform %[1]s: %[2]s"
 msgstr "installing platform %[1]s: %[2]s"
 
-#: arduino/discovery/discovery.go:184
+#: arduino/discovery/discovery.go:190
 msgid "invalid 'add' message: missing port"
 msgstr "invalid 'add' message: missing port"
 
-#: arduino/discovery/discovery.go:195
+#: arduino/discovery/discovery.go:201
 msgid "invalid 'remove' message: missing port"
 msgstr "invalid 'remove' message: missing port"
 
@@ -2840,7 +2840,7 @@ msgstr "library is not valid: missing header file \"%s\""
 msgid "library path does not exist: %s"
 msgstr "library path does not exist: %s"
 
-#: arduino/discovery/discoverymanager/discoverymanager.go:222
+#: arduino/discovery/discoverymanager/discoverymanager.go:225
 msgid "listing ports from discovery %[1]s: %[2]w"
 msgstr "listing ports from discovery %[1]s: %[2]w"
 
@@ -2938,7 +2938,7 @@ msgstr "no compatible version of %s tools found for the current os"
 msgid "no executable specified"
 msgstr "no executable specified"
 
-#: commands/daemon/daemon.go:94
+#: commands/daemon/daemon.go:96
 msgid "no instance specified"
 msgstr "no instance specified"
 
@@ -3050,7 +3050,7 @@ msgstr "port"
 msgid "port not found: %[1]s %[2]s"
 msgstr "port not found: %[1]s %[2]s"
 
-#: arduino/discovery/discovery.go:303
+#: arduino/discovery/discovery.go:306
 #: arduino/monitor/monitor.go:252
 msgid "protocol version not supported: requested 1, got %d"
 msgstr "protocol version not supported: requested 1, got %d"
@@ -3197,7 +3197,7 @@ msgstr "start syncing discovery %[1]s: %[2]w"
 msgid "starting discovery %[1]s: %[2]w"
 msgstr "starting discovery %[1]s: %[2]w"
 
-#: commands/board/list.go:283
+#: commands/board/list.go:302
 msgid "stopping discoveries: %s"
 msgstr "stopping discoveries: %s"
 
@@ -3233,7 +3233,7 @@ msgstr "the platform has no releases"
 msgid "the server responded with status %s"
 msgstr "the server responded with status %s"
 
-#: arduino/discovery/discovery.go:228
+#: arduino/discovery/discovery.go:231
 msgid "timeout waiting for message from %s"
 msgstr "timeout waiting for message from %s"