lib/api, lib/connections, gui: Show connection error for disconnected devices (fixes #3345) (#5727)

* lib/api, lib/connections, gui: Show connection error for disconnected devices (fixes #3345)

This adds functionality in the connetions service to track the last
error per address. That is in turn exposed in the /rest/system/status
API method, as that is also where we already show the listener status
from the connection service.

The GUI uses this info where it lists addresses, showing errors (if any)
in red underneath each address.

I also slightly refactored the existing status method on the connection
service to have a better name and return typed information.

* ok

* review

* formatting

* review

gui/default/index.html

@@ -715,8 +715,14 @@
                         </span>
                       </td>
                       <td ng-if="!connections[deviceCfg.deviceID].connected" class="text-right">
-                        <span ng-repeat="addr in deviceCfg.addresses"><span tooltip data-original-title="{{'Configured' | translate}}">{{addr}}</span><br></span>
+                        <span ng-repeat="addr in deviceCfg.addresses">
-                        <span ng-repeat="addr in discoveryCache[deviceCfg.deviceID].addresses"><span tooltip data-original-title="{{'Discovered' | translate}}">{{addr}}</span><br></span>
+                            <span tooltip data-original-title="{{'Configured' | translate}}">{{addr}}</span><br>
+                            <small ng-if="system.lastDialStatus[addr].error" tooltip data-original-title="{{system.lastDialStatus[addr].error}}" class="text-danger">{{abbreviatedError(addr)}}<br></small>
+                        </span>
+                        <span ng-repeat="addr in discoveryCache[deviceCfg.deviceID].addresses">
+                          <span tooltip data-original-title="{{'Discovered' | translate}}">{{addr}}</span><br>
+                          <small ng-if="system.lastDialStatus[addr].error" tooltip data-original-title="{{system.lastDialStatus[addr].error}}" class="text-danger">{{abbreviatedError(addr)}}<br></small>
+                        </span>
                       </td>
                     </tr>
                     <tr ng-if="connections[deviceCfg.deviceID].connected && connections[deviceCfg.deviceID].type.indexOf('Relay') > -1" tooltip data-original-title="Connections via relays might be rate limited by the relay">

gui/default/syncthing/core/syncthingController.js

@@ -2406,4 +2406,14 @@ angular.module('syncthing.core')
                 $scope.saveConfig();
             }
         };
+
+        $scope.abbreviatedError = function (addr) {
+            var status = $scope.system.lastDialStatus[addr];
+            if (!status || !status.error) {
+                return null;
+            }
+            var time = $filter('date')(status.when, "HH:mm:ss")
+            var err = status.error.replace(/.+: /, '');
+            return err + " (" + time + ")";
+        }
     });

lib/api/api.go

@@ -898,7 +898,8 @@ func (s *service) getSystemStatus(w http.ResponseWriter, r *http.Request) {
 		res["discoveryErrors"] = discoErrors
 	}
 
-	res["connectionServiceStatus"] = s.connectionsService.Status()
+	res["connectionServiceStatus"] = s.connectionsService.ListenerStatus()
+	res["lastDialStatus"] = s.connectionsService.ConnectionStatus()
 	// cpuUsage.Rate() is in milliseconds per second, so dividing by ten
 	// gives us percent
 	res["cpuPercent"] = s.cpu.Rate() / 10 / float64(runtime.NumCPU())

lib/api/mocked_connections_test.go

@@ -6,9 +6,17 @@
 
 package api
 
+import (
+	"github.com/syncthing/syncthing/lib/connections"
+)
+
 type mockedConnections struct{}
 
-func (m *mockedConnections) Status() map[string]interface{} {
+func (m *mockedConnections) ListenerStatus() map[string]connections.ListenerStatusEntry {
+	return nil
+}
+
+func (m *mockedConnections) ConnectionStatus() map[string]connections.ConnectionStatusEntry {
 	return nil
 }
 

lib/connections/service.go

@@ -90,10 +90,22 @@ var tlsVersionNames = map[uint16]string{
 // dialers. Successful connections are handed to the model.
 type Service interface {
 	suture.Service
-	Status() map[string]interface{}
+	ListenerStatus() map[string]ListenerStatusEntry
+	ConnectionStatus() map[string]ConnectionStatusEntry
 	NATType() string
 }
 
+type ListenerStatusEntry struct {
+	Error        *string  `json:"error"`
+	LANAddresses []string `json:"lanAddresses"`
+	WANAddresses []string `json:"wanAddresses"`
+}
+
+type ConnectionStatusEntry struct {
+	When  time.Time `json:"when"`
+	Error *string   `json:"error"`
+}
+
 type service struct {
 	*suture.Supervisor
 	cfg                  config.Wrapper
@@ -112,6 +124,9 @@ type service struct {
 	listeners          map[string]genericListener
 	listenerTokens     map[string]suture.ServiceToken
 	listenerSupervisor *suture.Supervisor
+
+	connectionStatusMut sync.RWMutex
+	connectionStatus    map[string]ConnectionStatusEntry // address -> latest error/status
 }
 
 func NewService(cfg config.Wrapper, myID protocol.DeviceID, mdl Model, tlsCfg *tls.Config, discoverer discover.Finder,
@@ -151,6 +166,9 @@ func NewService(cfg config.Wrapper, myID protocol.DeviceID, mdl Model, tlsCfg *t
 			FailureBackoff:    600 * time.Second,
 			PassThroughPanics: true,
 		}),
+
+		connectionStatusMut: sync.NewRWMutex(),
+		connectionStatus:    make(map[string]ConnectionStatusEntry),
 	}
 	cfg.Subscribe(service)
 
@@ -378,18 +396,23 @@ func (s *service) connect() {
 
 				uri, err := url.Parse(addr)
 				if err != nil {
+					s.setConnectionStatus(addr, err)
 					l.Infof("Parsing dialer address %s: %v", addr, err)
 					continue
 				}
 
 				if len(deviceCfg.AllowedNetworks) > 0 {
 					if !IsAllowedNetwork(uri.Host, deviceCfg.AllowedNetworks) {
+						s.setConnectionStatus(addr, errors.New("network disallowed"))
 						l.Debugln("Network for", uri, "is disallowed")
 						continue
 					}
 				}
 
 				dialerFactory, err := getDialerFactory(cfg, uri)
+				if err != nil {
+					s.setConnectionStatus(addr, err)
+				}
 				switch err {
 				case nil:
 					// all good
@@ -424,6 +447,7 @@ func (s *service) connect() {
 				}
 
 				dialTargets = append(dialTargets, dialTarget{
+					addr:     addr,
 					dialer:   dialer,
 					priority: priority,
 					deviceID: deviceID,
@@ -431,7 +455,7 @@ func (s *service) connect() {
 				})
 			}
 
-			conn, ok := dialParallel(deviceCfg.DeviceID, dialTargets)
+			conn, ok := s.dialParallel(deviceCfg.DeviceID, dialTargets)
 			if ok {
 				s.conns <- conn
 			}
@@ -633,19 +657,19 @@ func (s *service) ExternalAddresses() []string {
 	return util.UniqueStrings(addrs)
 }
 
-func (s *service) Status() map[string]interface{} {
+func (s *service) ListenerStatus() map[string]ListenerStatusEntry {
+	result := make(map[string]ListenerStatusEntry)
 	s.listenersMut.RLock()
-	result := make(map[string]interface{})
 	for addr, listener := range s.listeners {
-		status := make(map[string]interface{})
+		var status ListenerStatusEntry
 
-		err := listener.Error()
+		if err := listener.Error(); err != nil {
-		if err != nil {
+			errStr := err.Error()
-			status["error"] = err.Error()
+			status.Error = &errStr
 		}
 
-		status["lanAddresses"] = urlsToStrings(listener.LANAddresses())
+		status.LANAddresses = urlsToStrings(listener.LANAddresses())
-		status["wanAddresses"] = urlsToStrings(listener.WANAddresses())
+		status.WANAddresses = urlsToStrings(listener.WANAddresses())
 
 		result[addr] = status
 	}
@@ -653,6 +677,28 @@ func (s *service) Status() map[string]interface{} {
 	return result
 }
 
+func (s *service) ConnectionStatus() map[string]ConnectionStatusEntry {
+	result := make(map[string]ConnectionStatusEntry)
+	s.connectionStatusMut.RLock()
+	for k, v := range s.connectionStatus {
+		result[k] = v
+	}
+	s.connectionStatusMut.RUnlock()
+	return result
+}
+
+func (s *service) setConnectionStatus(address string, err error) {
+	status := ConnectionStatusEntry{When: time.Now().UTC().Truncate(time.Second)}
+	if err != nil {
+		errStr := err.Error()
+		status.Error = &errStr
+	}
+
+	s.connectionStatusMut.Lock()
+	s.connectionStatus[address] = status
+	s.connectionStatusMut.Unlock()
+}
+
 func (s *service) NATType() string {
 	s.listenersMut.RLock()
 	defer s.listenersMut.RUnlock()
@@ -769,7 +815,7 @@ func IsAllowedNetwork(host string, allowed []string) bool {
 	return false
 }
 
-func dialParallel(deviceID protocol.DeviceID, dialTargets []dialTarget) (internalConn, bool) {
+func (s *service) dialParallel(deviceID protocol.DeviceID, dialTargets []dialTarget) (internalConn, bool) {
 	// Group targets into buckets by priority
 	dialTargetBuckets := make(map[int][]dialTarget, len(dialTargets))
 	for _, tgt := range dialTargets {
@@ -793,6 +839,7 @@ func dialParallel(deviceID protocol.DeviceID, dialTargets []dialTarget) (interna
 			wg.Add(1)
 			go func(tgt dialTarget) {
 				conn, err := tgt.Dial()
+				s.setConnectionStatus(tgt.addr, err)
 				if err == nil {
 					res <- conn
 				}

lib/connections/structs.go

@@ -195,6 +195,7 @@ func (o *onAddressesChangedNotifier) notifyAddressesChanged(l genericListener) {
 }
 
 type dialTarget struct {
+	addr     string
 	dialer   genericDialer
 	priority int
 	uri      *url.URL