Break out UPnP port mapping into a service

cmd/syncthing/main.go

@@ -35,7 +35,6 @@ import (
 	"github.com/syncthing/syncthing/internal/osutil"
 	"github.com/syncthing/syncthing/internal/symlinks"
 	"github.com/syncthing/syncthing/internal/upgrade"
-	"github.com/syncthing/syncthing/internal/upnp"
 	"github.com/syndtr/goleveldb/leveldb"
 	"github.com/syndtr/goleveldb/leveldb/errors"
 	"github.com/syndtr/goleveldb/leveldb/opt"
@@ -109,8 +108,6 @@ var (
 	readRateLimit  *ratelimit.Bucket
 	stop           = make(chan int)
 	discoverer     *discover.Discoverer
-	externalPort   int
-	igd            *upnp.IGD
 	cert           tls.Certificate
 	lans           []*net.IPNet
 )
@@ -573,18 +570,20 @@ func syncthingMain() {
 	if err != nil {
 		l.Fatalln("Bad listen address:", err)
 	}
-	externalPort = addr.Port
 
-	// UPnP
-	igd = nil
+	// Start discovery
+
+	localPort := addr.Port
+	discoverer = discovery(localPort)
+
+	// Start UPnP. The UPnP service will restart global discovery if the
+	// external port changes.
 
 	if opts.UPnPEnabled {
-		setupUPnP()
+		upnpSvc := newUPnPSvc(cfg, localPort)
+		mainSvc.Add(upnpSvc)
 	}
 
-	// Routine to connect out to configured devices
-	discoverer = discovery(externalPort)
-
 	connectionSvc := newConnectionSvc(cfg, myID, m, tlsCfg)
 	mainSvc.Add(connectionSvc)
 
@@ -765,110 +764,6 @@ func generatePingEvents() {
 	}
 }
 
-func setupUPnP() {
-	if opts := cfg.Options(); len(opts.ListenAddress) == 1 {
-		_, portStr, err := net.SplitHostPort(opts.ListenAddress[0])
-		if err != nil {
-			l.Warnln("Bad listen address:", err)
-		} else {
-			// Set up incoming port forwarding, if necessary and possible
-			port, _ := strconv.Atoi(portStr)
-			igds := upnp.Discover(time.Duration(cfg.Options().UPnPTimeoutS) * time.Second)
-			if len(igds) > 0 {
-				// Configure the first discovered IGD only. This is a work-around until we have a better mechanism
-				// for handling multiple IGDs, which will require changes to the global discovery service
-				igd = &igds[0]
-
-				externalPort = setupExternalPort(igd, port)
-				if externalPort == 0 {
-					l.Warnln("Failed to create UPnP port mapping")
-				} else {
-					l.Infof("Created UPnP port mapping for external port %d on UPnP device %s.", externalPort, igd.FriendlyIdentifier())
-
-					if opts.UPnPRenewalM > 0 {
-						go renewUPnP(port)
-					}
-				}
-			}
-		}
-	} else {
-		l.Warnln("Multiple listening addresses; not attempting UPnP port mapping")
-	}
-}
-
-func setupExternalPort(igd *upnp.IGD, port int) int {
-	if igd == nil {
-		return 0
-	}
-
-	for i := 0; i < 10; i++ {
-		r := 1024 + predictableRandom.Intn(65535-1024)
-		err := igd.AddPortMapping(upnp.TCP, r, port, fmt.Sprintf("syncthing-%d", r), cfg.Options().UPnPLeaseM*60)
-		if err == nil {
-			return r
-		}
-	}
-	return 0
-}
-
-func renewUPnP(port int) {
-	for {
-		opts := cfg.Options()
-		time.Sleep(time.Duration(opts.UPnPRenewalM) * time.Minute)
-		// Some values might have changed while we were sleeping
-		opts = cfg.Options()
-
-		// Make sure our IGD reference isn't nil
-		if igd == nil {
-			if debugNet {
-				l.Debugln("Undefined IGD during UPnP port renewal. Re-discovering...")
-			}
-			igds := upnp.Discover(time.Duration(opts.UPnPTimeoutS) * time.Second)
-			if len(igds) > 0 {
-				// Configure the first discovered IGD only. This is a work-around until we have a better mechanism
-				// for handling multiple IGDs, which will require changes to the global discovery service
-				igd = &igds[0]
-			} else {
-				if debugNet {
-					l.Debugln("Failed to discover IGD during UPnP port mapping renewal.")
-				}
-				continue
-			}
-		}
-
-		// Just renew the same port that we already have
-		if externalPort != 0 {
-			err := igd.AddPortMapping(upnp.TCP, externalPort, port, "syncthing", opts.UPnPLeaseM*60)
-			if err != nil {
-				l.Warnf("Error renewing UPnP port mapping for external port %d on device %s: %s", externalPort, igd.FriendlyIdentifier(), err.Error())
-			} else if debugNet {
-				l.Debugf("Renewed UPnP port mapping for external port %d on device %s.", externalPort, igd.FriendlyIdentifier())
-			}
-
-			continue
-		}
-
-		// Something strange has happened. We didn't have an external port before?
-		// Or perhaps the gateway has changed?
-		// Retry the same port sequence from the beginning.
-		if debugNet {
-			l.Debugln("No UPnP port mapping defined, updating...")
-		}
-
-		forwardedPort := setupExternalPort(igd, port)
-		if forwardedPort != 0 {
-			externalPort = forwardedPort
-			discoverer.StopGlobal()
-			discoverer.StartGlobal(opts.GlobalAnnServers, uint16(forwardedPort))
-			if debugNet {
-				l.Debugf("Updated UPnP port mapping for external port %d on device %s.", forwardedPort, igd.FriendlyIdentifier())
-			}
-		} else {
-			l.Warnf("Failed to update UPnP port mapping for external port on device " + igd.FriendlyIdentifier() + ".")
-		}
-	}
-}
-
 func resetDB() error {
 	return os.RemoveAll(locations[locDatabase])
 }

cmd/syncthing/upnpsvc.go

@@ -0,0 +1,111 @@
+// Copyright (C) 2015 The Syncthing Authors.
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at http://mozilla.org/MPL/2.0/.
+
+package main
+
+import (
+	"fmt"
+	"time"
+
+	"github.com/syncthing/syncthing/internal/config"
+	"github.com/syncthing/syncthing/internal/upnp"
+)
+
+// The UPnP service runs a loop for discovery of IGDs (Internet Gateway
+// Devices) and setup/renewal of a port mapping.
+type upnpSvc struct {
+	cfg       *config.Wrapper
+	localPort int
+}
+
+func newUPnPSvc(cfg *config.Wrapper, localPort int) *upnpSvc {
+	return &upnpSvc{
+		cfg:       cfg,
+		localPort: localPort,
+	}
+}
+
+func (s *upnpSvc) Serve() {
+	extPort := 0
+	foundIGD := true
+
+	for {
+		igds := upnp.Discover(time.Duration(s.cfg.Options().UPnPTimeoutS) * time.Second)
+		if len(igds) > 0 {
+			foundIGD = true
+			extPort = s.tryIGDs(igds, extPort)
+		} else if foundIGD {
+			// Only print a notice if we've previously found an IGD or this is
+			// the first time around.
+			foundIGD = false
+			l.Infof("No UPnP device detected")
+		}
+
+		d := time.Duration(s.cfg.Options().UPnPRenewalM) * time.Minute
+		if d == 0 {
+			// We always want to do renewal so lets just pick a nice sane number.
+			d = 30 * time.Minute
+		}
+		time.Sleep(d)
+	}
+}
+
+func (s *upnpSvc) Stop() {
+	panic("upnpSvc cannot stop")
+}
+
+func (s *upnpSvc) tryIGDs(igds []upnp.IGD, prevExtPort int) int {
+	// Lets try all the IGDs we found and use the first one that works.
+	// TODO: Use all of them, and sort out the resulting mess to the
+	// discovery announcement code...
+	for _, igd := range igds {
+		extPort, err := s.tryIGD(igd, prevExtPort)
+		if err != nil {
+			l.Warnf("Failed to set UPnP port mapping: external port %d on device %s.", extPort, igd.FriendlyIdentifier())
+			continue
+		}
+
+		if extPort != prevExtPort {
+			// External port changed; refresh the discovery announcement.
+			// TODO: Don't reach out to some magic global here?
+			l.Infof("New UPnP port mapping: external port %d to local port %d.", extPort, s.localPort)
+			discoverer.StopGlobal()
+			discoverer.StartGlobal(s.cfg.Options().GlobalAnnServers, uint16(extPort))
+		}
+		if debugNet {
+			l.Debugf("Created/updated UPnP port mapping for external port %d on device %s.", extPort, igd.FriendlyIdentifier())
+		}
+		return extPort
+	}
+
+	return 0
+}
+
+func (s *upnpSvc) tryIGD(igd upnp.IGD, suggestedPort int) (int, error) {
+	var err error
+	leaseTime := s.cfg.Options().UPnPLeaseM * 60
+
+	if suggestedPort != 0 {
+		// First try renewing our existing mapping.
+		name := fmt.Sprintf("syncthing-%d", suggestedPort)
+		err = igd.AddPortMapping(upnp.TCP, suggestedPort, s.localPort, name, leaseTime)
+		if err == nil {
+			return suggestedPort, nil
+		}
+	}
+
+	for i := 0; i < 10; i++ {
+		// Then try up to ten random ports.
+		extPort := 1024 + predictableRandom.Intn(65535-1024)
+		name := fmt.Sprintf("syncthing-%d", suggestedPort)
+		err = igd.AddPortMapping(upnp.TCP, extPort, s.localPort, name, leaseTime)
+		if err == nil {
+			return extPort, nil
+		}
+	}
+
+	return 0, err
+}

internal/config/config.go

@@ -230,7 +230,7 @@ type OptionsConfiguration struct {
 	UPnPEnabled             bool     `xml:"upnpEnabled" json:"upnpEnabled" default:"true"`
 	UPnPLeaseM              int      `xml:"upnpLeaseMinutes" json:"upnpLeaseMinutes" default:"0"`
 	UPnPRenewalM            int      `xml:"upnpRenewalMinutes" json:"upnpRenewalMinutes" default:"30"`
-	UPnPTimeoutS            int      `xml:"upnpTimeoutSeconds" json:"upnpTimeoutSeconds" default:"3"`
+	UPnPTimeoutS            int      `xml:"upnpTimeoutSeconds" json:"upnpTimeoutSeconds" default:"10"`
 	URAccepted              int      `xml:"urAccepted" json:"urAccepted"` // Accepted usage reporting version; 0 for off (undecided), -1 for off (permanently)
 	URUniqueID              string   `xml:"urUniqueID" json:"urUniqueId"` // Unique ID for reporting purposes, regenerated when UR is turned on.
 	RestartOnWakeup         bool     `xml:"restartOnWakeup" json:"restartOnWakeup" default:"true"`

internal/config/config_test.go

@@ -44,7 +44,7 @@ func TestDefaultValues(t *testing.T) {
 		UPnPEnabled:             true,
 		UPnPLeaseM:              0,
 		UPnPRenewalM:            30,
-		UPnPTimeoutS:            3,
+		UPnPTimeoutS:            10,
 		RestartOnWakeup:         true,
 		AutoUpgradeIntervalH:    12,
 		KeepTemporariesH:        24,

internal/upnp/upnp.go

@@ -95,7 +95,6 @@ type upnpRoot struct {
 // The order in which the devices appear in the results list is not deterministic.
 func Discover(timeout time.Duration) []IGD {
 	var results []IGD
-	l.Infoln("Starting UPnP discovery...")
 
 	interfaces, err := net.Interfaces()
 	if err != nil {
@@ -144,13 +143,6 @@ func Discover(timeout time.Duration) []IGD {
 	wg.Wait()
 	close(resultChan)
 
-	suffix := "devices"
-	if len(results) == 1 {
-		suffix = "device"
-	}
-
-	l.Infof("UPnP discovery complete (found %d %s).", len(results), suffix)
-
 	return results
 }
 

test/h2/config.xml

@@ -54,9 +54,9 @@
         <maxRecvKbps>0</maxRecvKbps>
         <reconnectionIntervalS>5</reconnectionIntervalS>
         <startBrowser>false</startBrowser>
-        <upnpEnabled>false</upnpEnabled>
+        <upnpEnabled>true</upnpEnabled>
         <upnpLeaseMinutes>0</upnpLeaseMinutes>
-        <upnpRenewalMinutes>30</upnpRenewalMinutes>
+        <upnpRenewalMinutes>1</upnpRenewalMinutes>
         <urAccepted>-1</urAccepted>
         <urUniqueID></urUniqueID>
         <restartOnWakeup>true</restartOnWakeup>