all: Don't let Suture capture panics (fixes #4758) (#5119)

Fork with new option.

cmd/stdiscosrv/main.go

@@ -19,11 +19,11 @@ import (
 	"strings"
 	"time"
 
+	"github.com/calmh/suture"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	"github.com/syncthing/syncthing/lib/protocol"
 	"github.com/syncthing/syncthing/lib/tlsutil"
 	"github.com/syndtr/goleveldb/leveldb/opt"
-	"github.com/thejerf/suture"
 )
 
 const (
@@ -164,7 +164,9 @@ func main() {
 	}
 
 	// Root of the service tree.
-	main := suture.NewSimple("main")
+	main := suture.New("main", suture.Spec{
+		PanicPanics: true,
+	})
 
 	// Start the database.
 	db, err := newLevelDBStore(dir)

cmd/syncthing/gui_test.go

@@ -23,13 +23,13 @@ import (
 	"testing"
 	"time"
 
+	"github.com/calmh/suture"
 	"github.com/d4l3k/messagediff"
 	"github.com/syncthing/syncthing/lib/config"
 	"github.com/syncthing/syncthing/lib/events"
 	"github.com/syncthing/syncthing/lib/fs"
 	"github.com/syncthing/syncthing/lib/protocol"
 	"github.com/syncthing/syncthing/lib/sync"
-	"github.com/thejerf/suture"
 )
 
 func TestCSRFToken(t *testing.T) {
@@ -77,7 +77,9 @@ func TestStopAfterBrokenConfig(t *testing.T) {
 	srv := newAPIService(protocol.LocalDeviceID, w, "../../test/h1/https-cert.pem", "../../test/h1/https-key.pem", "", nil, nil, nil, nil, nil, nil, nil, nil)
 	srv.started = make(chan string)
 
-	sup := suture.NewSimple("test")
+	sup := suture.New("test", suture.Spec{
+		PanicPanics: true,
+	})
 	sup.Add(srv)
 	sup.ServeBackground()
 
@@ -487,7 +489,9 @@ func startHTTP(cfg *mockedConfig) (string, error) {
 	svc.started = addrChan
 
 	// Actually start the API service
-	supervisor := suture.NewSimple("API test")
+	supervisor := suture.New("API test", suture.Spec{
+		PanicPanics: true,
+	})
 	supervisor.Add(svc)
 	supervisor.ServeBackground()
 

cmd/syncthing/main.go

@@ -47,7 +47,7 @@ import (
 	"github.com/syncthing/syncthing/lib/tlsutil"
 	"github.com/syncthing/syncthing/lib/upgrade"
 
-	"github.com/thejerf/suture"
+	"github.com/calmh/suture"
 
 	_ "net/http/pprof" // Need to import this to support STPROFILER.
 )
@@ -594,6 +594,7 @@ func syncthingMain(runtimeOptions RuntimeOptions) {
 		Log: func(line string) {
 			l.Debugln(line)
 		},
+		PanicPanics: true,
 	})
 	mainService.ServeBackground()
 

cmd/syncthing/summaryservice.go

@@ -9,10 +9,10 @@ package main
 import (
 	"time"
 
+	"github.com/calmh/suture"
 	"github.com/syncthing/syncthing/lib/events"
 	"github.com/syncthing/syncthing/lib/protocol"
 	"github.com/syncthing/syncthing/lib/sync"
-	"github.com/thejerf/suture"
 )
 
 // The folderSummaryService adds summary information events (FolderSummary and
@@ -36,7 +36,9 @@ type folderSummaryService struct {
 
 func newFolderSummaryService(cfg configIntf, m modelIntf) *folderSummaryService {
 	service := &folderSummaryService{
-		Supervisor:      suture.NewSimple("folderSummaryService"),
+		Supervisor: suture.New("folderSummaryService", suture.Spec{
+			PanicPanics: true,
+		}),
 		cfg:             cfg,
 		model:           m,
 		stop:            make(chan struct{}),

lib/beacon/beacon.go

@@ -10,7 +10,7 @@ import (
 	"net"
 	stdsync "sync"
 
-	"github.com/thejerf/suture"
+	"github.com/calmh/suture"
 )
 
 type recv struct {

lib/beacon/broadcast.go

@@ -11,8 +11,8 @@ import (
 	"net"
 	"time"
 
+	"github.com/calmh/suture"
 	"github.com/syncthing/syncthing/lib/sync"
-	"github.com/thejerf/suture"
 )
 
 type Broadcast struct {
@@ -36,6 +36,7 @@ func NewBroadcast(port int) *Broadcast {
 			Log: func(line string) {
 				l.Debugln(line)
 			},
+			PanicPanics: true,
 		}),
 		port:   port,
 		inbox:  make(chan []byte),

lib/beacon/multicast.go

@@ -12,7 +12,7 @@ import (
 	"net"
 	"time"
 
-	"github.com/thejerf/suture"
+	"github.com/calmh/suture"
 	"golang.org/x/net/ipv6"
 )
 
@@ -36,6 +36,7 @@ func NewMulticast(addr string) *Multicast {
 			Log: func(line string) {
 				l.Debugln(line)
 			},
+			PanicPanics: true,
 		}),
 		inbox:  make(chan []byte),
 		outbox: make(chan recv, 16),

lib/connections/service.go

@@ -30,7 +30,7 @@ import (
 	_ "github.com/syncthing/syncthing/lib/pmp"
 	_ "github.com/syncthing/syncthing/lib/upnp"
 
-	"github.com/thejerf/suture"
+	"github.com/calmh/suture"
 	"golang.org/x/time/rate"
 )
 
@@ -105,6 +105,7 @@ func NewService(cfg *config.Wrapper, myID protocol.DeviceID, mdl Model, tlsCfg *
 			Log: func(line string) {
 				l.Infoln(line)
 			},
+			PanicPanics: true,
 		}),
 		cfg:                  cfg,
 		myID:                 myID,
@@ -131,6 +132,7 @@ func NewService(cfg *config.Wrapper, myID protocol.DeviceID, mdl Model, tlsCfg *
 			},
 			FailureThreshold: 2,
 			FailureBackoff:   600 * time.Second,
+			PanicPanics:      true,
 		}),
 	}
 	cfg.Subscribe(service)

lib/discover/cache.go

@@ -10,10 +10,10 @@ import (
 	stdsync "sync"
 	"time"
 
+	"github.com/calmh/suture"
 	"github.com/syncthing/syncthing/lib/protocol"
 	"github.com/syncthing/syncthing/lib/sync"
 	"github.com/syncthing/syncthing/lib/util"
-	"github.com/thejerf/suture"
 )
 
 // The CachingMux aggregates results from multiple Finders. Each Finder has
@@ -51,8 +51,10 @@ type cachedError interface {
 
 func NewCachingMux() CachingMux {
 	return &cachingMux{
-		Supervisor: suture.NewSimple("discover.cachingMux"),
+		Supervisor: suture.New("discover.cachingMux", suture.Spec{
-		mut:        sync.NewRWMutex(),
+			PanicPanics: true,
+		}),
+		mut: sync.NewRWMutex(),
 	}
 }
 

lib/discover/discover.go

@@ -9,8 +9,8 @@ package discover
 import (
 	"time"
 
+	"github.com/calmh/suture"
 	"github.com/syncthing/syncthing/lib/protocol"
-	"github.com/thejerf/suture"
 )
 
 // A Finder provides lookup services of some kind.

lib/discover/local.go

@@ -18,11 +18,11 @@ import (
 	"strconv"
 	"time"
 
+	"github.com/calmh/suture"
 	"github.com/syncthing/syncthing/lib/beacon"
 	"github.com/syncthing/syncthing/lib/events"
 	"github.com/syncthing/syncthing/lib/protocol"
 	"github.com/syncthing/syncthing/lib/rand"
-	"github.com/thejerf/suture"
 )
 
 type localClient struct {
@@ -48,7 +48,9 @@ const (
 
 func NewLocal(id protocol.DeviceID, addr string, addrList AddressLister) (FinderService, error) {
 	c := &localClient{
-		Supervisor:      suture.NewSimple("local"),
+		Supervisor: suture.New("local", suture.Spec{
+			PanicPanics: true,
+		}),
 		myID:            id,
 		addrList:        addrList,
 		localBcastTick:  time.NewTicker(BroadcastInterval).C,

lib/model/model.go

@@ -22,6 +22,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/calmh/suture"
 	"github.com/syncthing/syncthing/lib/config"
 	"github.com/syncthing/syncthing/lib/connections"
 	"github.com/syncthing/syncthing/lib/db"
@@ -35,7 +36,6 @@ import (
 	"github.com/syncthing/syncthing/lib/sync"
 	"github.com/syncthing/syncthing/lib/upgrade"
 	"github.com/syncthing/syncthing/lib/versioner"
-	"github.com/thejerf/suture"
 )
 
 var locationLocal *time.Location
@@ -136,6 +136,7 @@ func NewModel(cfg *config.Wrapper, id protocol.DeviceID, clientName, clientVersi
 			Log: func(line string) {
 				l.Debugln(line)
 			},
+			PanicPanics: true,
 		}),
 		cfg:                 cfg,
 		db:                  ldb,

vendor/github.com/calmh/suture/messages.go

@@ -46,7 +46,7 @@ func (s *Supervisor) serviceEnded(id serviceID, complete bool) {
 }
 
 type serviceEnded struct {
-	id serviceID
+	id       serviceID
 	complete bool
 }
 

vendor/github.com/calmh/suture/supervisor.go

@@ -108,6 +108,7 @@ type Supervisor struct {
 	control              chan supervisorMessage
 	liveness             chan struct{}
 	resumeTimer          <-chan time.Time
+	recoverPanics        bool
 
 	LogBadStop BadStopLogger
 	LogFailure FailureLogger
@@ -133,6 +134,7 @@ type Spec struct {
 	LogBadStop       BadStopLogger
 	LogFailure       FailureLogger
 	LogBackoff       BackoffLogger
+	PanicPanics      bool
 }
 
 /*
@@ -214,6 +216,7 @@ func New(name string, spec Spec) (s *Supervisor) {
 	} else {
 		s.timeout = spec.Timeout
 	}
+	s.recoverPanics = !spec.PanicPanics
 
 	// overriding these allows for testing the threshold behavior
 	s.getNow = time.Now
@@ -520,14 +523,16 @@ func (s *Supervisor) handleFailedService(id serviceID, err interface{}, stacktra
 
 func (s *Supervisor) runService(service Service, id serviceID) {
 	go func() {
-		defer func() {
+		if s.recoverPanics {
-			if r := recover(); r != nil {
+			defer func() {
-				buf := make([]byte, 65535, 65535)
+				if r := recover(); r != nil {
-				written := runtime.Stack(buf, false)
+					buf := make([]byte, 65535, 65535)
-				buf = buf[:written]
+					written := runtime.Stack(buf, false)
-				s.fail(id, r, buf)
+					buf = buf[:written]
-			}
+					s.fail(id, r, buf)
-		}()
+				}
+			}()
+		}
 
 		service.Serve()
 
@@ -639,7 +644,10 @@ RemoveAndWait will remove the given service from the Supervisor and attempt
 to Stop() it. It will wait up to the given timeout value for the service to
 terminate. A timeout value of 0 means to wait forever.
 
-If a nil error is returned from this function
+If a nil error is returned from this function, then the service was
+terminated normally. If either the supervisor terminates or the timeout
+passes, ErrTimeout is returned. (If this isn't even the right supervisor
+ErrWrongSupervisor is returned.)
 */
 func (s *Supervisor) RemoveAndWait(id ServiceToken, timeout time.Duration) error {
 	sID := supervisorID(id.id >> 32)

vendor/manifest

@@ -74,6 +74,14 @@
 			"branch": "master",
 			"notests": true
 		},
+		{
+			"importpath": "github.com/calmh/suture",
+			"repository": "https://github.com/calmh/suture",
+			"vcs": "git",
+			"revision": "2741a6bb8fdeba8f30c948c83756edc4dd21b9c6",
+			"branch": "master",
+			"notests": true
+		},
 		{
 			"importpath": "github.com/calmh/xdr",
 			"repository": "https://github.com/calmh/xdr",
@@ -432,14 +440,6 @@
 			"branch": "master",
 			"notests": true
 		},
-		{
-			"importpath": "github.com/thejerf/suture",
-			"repository": "https://github.com/thejerf/suture",
-			"vcs": "git",
-			"revision": "3f1fb62fe0a3cc6429122d7dc45588a8b59c5bb6",
-			"branch": "master",
-			"notests": true
-		},
 		{
 			"importpath": "github.com/tjfoc/gmsm/sm4",
 			"repository": "https://github.com/tjfoc/gmsm",