lib/model: Use a single lock (phase two: cleanup) (#9276)

Cleanup after #9275.

This renames `fmut` -> `mut`, removes the deadlock detector and
associated plumbing, renames some things from `...PRLocked` to
`...RLocked` and similar, and updates comments.

Apart from the removal of the deadlock detection machinery, no
functional code changes... i.e. almost 100% diff noise, have fun
reviewing.

cmd/syncthing/main.go

@@ -88,9 +88,6 @@ above.
  STTRACE           A comma separated string of facilities to trace. The valid
                    facility strings are listed below.
 
- STDEADLOCKTIMEOUT Used for debugging internal deadlocks; sets debug
-                   sensitivity. Use only under direction of a developer.
-
  STLOCKTHRESHOLD   Used for debugging internal deadlocks; sets debug
                    sensitivity.  Use only under direction of a developer.
 
@@ -173,7 +170,6 @@ type serveOptions struct {
 	// Debug options below
 	DebugDBIndirectGCInterval time.Duration `env:"STGCINDIRECTEVERY" help:"Database indirection GC interval"`
 	DebugDBRecheckInterval    time.Duration `env:"STRECHECKDBEVERY" help:"Database metadata recalculation interval"`
-	DebugDeadlockTimeout      int           `placeholder:"SECONDS" env:"STDEADLOCKTIMEOUT" help:"Used for debugging internal deadlocks"`
 	DebugGUIAssetsDir         string        `placeholder:"PATH" help:"Directory to load GUI assets from" env:"STGUIASSETS"`
 	DebugPerfStats            bool          `env:"STPERFSTATS" help:"Write running performance statistics to perf-$pid.csv (Unix only)"`
 	DebugProfileBlock         bool          `env:"STBLOCKPROFILE" help:"Write block profiles to block-$pid-$timestamp.pprof every 20 seconds"`
@@ -623,7 +619,6 @@ func syncthingMain(options serveOptions) {
 	}
 
 	appOpts := syncthing.Options{
-		DeadlockTimeoutS:     options.DebugDeadlockTimeout,
 		NoUpgrade:            options.NoUpgrade,
 		ProfilerAddr:         options.DebugProfilerListen,
 		ResetDeltaIdxs:       options.DebugResetDeltaIdxs,
@@ -634,10 +629,6 @@ func syncthingMain(options serveOptions) {
 	if options.Audit {
 		appOpts.AuditWriter = auditWriter(options.AuditFile)
 	}
-	if t := os.Getenv("STDEADLOCKTIMEOUT"); t != "" {
-		secs, _ := strconv.Atoi(t)
-		appOpts.DeadlockTimeoutS = secs
-	}
 	if dur, err := time.ParseDuration(os.Getenv("STRECHECKDBEVERY")); err == nil {
 		appOpts.DBRecheckInterval = dur
 	}

go.mod

@@ -40,7 +40,6 @@ require (
 	github.com/prometheus/procfs v0.12.0 // indirect
 	github.com/quic-go/quic-go v0.40.0
 	github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475
-	github.com/sasha-s/go-deadlock v0.3.1
 	github.com/shirou/gopsutil/v3 v3.23.11
 	github.com/syncthing/notify v0.0.0-20210616190510-c6b7342338d2
 	github.com/syndtr/goleveldb v1.0.1-0.20220721030215-126854af5e6d
@@ -68,7 +67,6 @@ require (
 	github.com/matttproud/golang_protobuf_extensions/v2 v2.0.0 // indirect
 	github.com/onsi/ginkgo/v2 v2.13.2 // indirect
 	github.com/oschwald/maxminddb-golang v1.12.0 // indirect
-	github.com/petermattis/goid v0.0.0-20231126143041-f558c26febf5 // indirect
 	github.com/power-devops/perfstat v0.0.0-20221212215047-62379fc7944b // indirect
 	github.com/prometheus/client_model v0.5.0 // indirect
 	github.com/quic-go/qtls-go1-20 v0.4.1 // indirect

go.sum

@@ -139,9 +139,6 @@ github.com/oschwald/geoip2-golang v1.9.0 h1:uvD3O6fXAXs+usU+UGExshpdP13GAqp4GBrz
 github.com/oschwald/geoip2-golang v1.9.0/go.mod h1:BHK6TvDyATVQhKNbQBdrj9eAvuwOMi2zSFXizL3K81Y=
 github.com/oschwald/maxminddb-golang v1.12.0 h1:9FnTOD0YOhP7DGxGsq4glzpGy5+w7pq50AS6wALUMYs=
 github.com/oschwald/maxminddb-golang v1.12.0/go.mod h1:q0Nob5lTCqyQ8WT6FYgS1L7PXKVVbgiymefNwIjPzgY=
-github.com/petermattis/goid v0.0.0-20180202154549-b0b1615b78e5/go.mod h1:jvVRKCrJTQWu0XVbaOlby/2lO20uSCHEMzzplHXte1o=
-github.com/petermattis/goid v0.0.0-20231126143041-f558c26febf5 h1:+qIP3OMrT7SN5kLnTcVEISPOMB/97RyAKTg1UWA738E=
-github.com/petermattis/goid v0.0.0-20231126143041-f558c26febf5/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4=
 github.com/pierrec/lz4/v4 v4.1.18 h1:xaKrnTkyoqfh1YItXl56+6KJNVYWlEEPuAQW9xsplYQ=
 github.com/pierrec/lz4/v4 v4.1.18/go.mod h1:gZWDp/Ze/IJXGXf23ltt2EXimqmTUXEy0GFuRQyBid4=
 github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
@@ -168,8 +165,6 @@ github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475 h1:N/ElC8H3+5X
 github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475/go.mod h1:bCqnVzQkZxMG4s8nGwiZ5l3QUCyqpo9Y+/ZMZ9VjZe4=
 github.com/russross/blackfriday/v2 v2.1.0 h1:JIOH55/0cWyOuilr9/qlrm0BSXldqnqwMsf35Ld67mk=
 github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
-github.com/sasha-s/go-deadlock v0.3.1 h1:sqv7fDNShgjcaxkO0JNcOAlr8B9+cV5Ey/OB71efZx0=
-github.com/sasha-s/go-deadlock v0.3.1/go.mod h1:F73l+cr82YSh10GxyRI6qZiCgK64VaZjwesgfQ1/iLM=
 github.com/sclevine/spec v1.4.0 h1:z/Q9idDcay5m5irkZ28M7PtQM4aOISzOpj4bUPkDee8=
 github.com/shirou/gopsutil/v3 v3.23.11 h1:i3jP9NjCPUz7FiZKxlMnODZkdSIp2gnzfrvsu9CuWEQ=
 github.com/shirou/gopsutil/v3 v3.23.11/go.mod h1:1FrWgea594Jp7qmjHUUPlJDTPgcsb9mGnXDxavtikzM=

lib/model/folder_recvonly_test.go

@@ -535,8 +535,8 @@ func setupROFolder(t *testing.T) (*testModel, *receiveOnlyFolder, context.Cancel
 	<-m.started
 	must(t, m.ScanFolder("ro"))
 
-	m.fmut.RLock()
-	defer m.fmut.RUnlock()
+	m.mut.RLock()
+	defer m.mut.RUnlock()
 	r, _ := m.folderRunners.Get("ro")
 	f := r.(*receiveOnlyFolder)
 

lib/model/mocks/model.go

@@ -531,11 +531,6 @@ type Model struct {
 	setIgnoresReturnsOnCall map[int]struct {
 		result1 error
 	}
-	StartDeadlockDetectorStub        func(time.Duration)
-	startDeadlockDetectorMutex       sync.RWMutex
-	startDeadlockDetectorArgsForCall []struct {
-		arg1 time.Duration
-	}
 	StateStub        func(string) (string, time.Time, error)
 	stateMutex       sync.RWMutex
 	stateArgsForCall []struct {
@@ -3070,38 +3065,6 @@ func (fake *Model) SetIgnoresReturnsOnCall(i int, result1 error) {
 	}{result1}
 }
 
-func (fake *Model) StartDeadlockDetector(arg1 time.Duration) {
-	fake.startDeadlockDetectorMutex.Lock()
-	fake.startDeadlockDetectorArgsForCall = append(fake.startDeadlockDetectorArgsForCall, struct {
-		arg1 time.Duration
-	}{arg1})
-	stub := fake.StartDeadlockDetectorStub
-	fake.recordInvocation("StartDeadlockDetector", []interface{}{arg1})
-	fake.startDeadlockDetectorMutex.Unlock()
-	if stub != nil {
-		fake.StartDeadlockDetectorStub(arg1)
-	}
-}
-
-func (fake *Model) StartDeadlockDetectorCallCount() int {
-	fake.startDeadlockDetectorMutex.RLock()
-	defer fake.startDeadlockDetectorMutex.RUnlock()
-	return len(fake.startDeadlockDetectorArgsForCall)
-}
-
-func (fake *Model) StartDeadlockDetectorCalls(stub func(time.Duration)) {
-	fake.startDeadlockDetectorMutex.Lock()
-	defer fake.startDeadlockDetectorMutex.Unlock()
-	fake.StartDeadlockDetectorStub = stub
-}
-
-func (fake *Model) StartDeadlockDetectorArgsForCall(i int) time.Duration {
-	fake.startDeadlockDetectorMutex.RLock()
-	defer fake.startDeadlockDetectorMutex.RUnlock()
-	argsForCall := fake.startDeadlockDetectorArgsForCall[i]
-	return argsForCall.arg1
-}
-
 func (fake *Model) State(arg1 string) (string, time.Time, error) {
 	fake.stateMutex.Lock()
 	ret, specificReturn := fake.stateReturnsOnCall[len(fake.stateArgsForCall)]
@@ -3351,8 +3314,6 @@ func (fake *Model) Invocations() map[string][][]interface{} {
 	defer fake.serveMutex.RUnlock()
 	fake.setIgnoresMutex.RLock()
 	defer fake.setIgnoresMutex.RUnlock()
-	fake.startDeadlockDetectorMutex.RLock()
-	defer fake.startDeadlockDetectorMutex.RUnlock()
 	fake.stateMutex.RLock()
 	defer fake.stateMutex.RUnlock()
 	fake.usageReportingStatsMutex.RLock()

lib/model/model_test.go

@@ -902,13 +902,13 @@ func TestIssue5063(t *testing.T) {
 	defer cleanupModel(m)
 	defer cancel()
 
-	m.fmut.Lock()
+	m.mut.Lock()
 	for _, c := range m.connections {
 		conn := c.(*fakeConnection)
 		conn.CloseCalls(func(_ error) {})
 		defer m.Closed(c, errStopped) // to unblock deferred m.Stop()
 	}
-	m.fmut.Unlock()
+	m.mut.Unlock()
 
 	wg := sync.WaitGroup{}
 
@@ -1524,10 +1524,10 @@ func TestIgnores(t *testing.T) {
 		FilesystemType: fs.FilesystemTypeFake,
 	}
 	ignores := ignore.New(fcfg.Filesystem(nil), ignore.WithCache(m.cfg.Options().CacheIgnoredFiles))
-	m.fmut.Lock()
+	m.mut.Lock()
 	m.folderCfgs[fcfg.ID] = fcfg
 	m.folderIgnores[fcfg.ID] = ignores
-	m.fmut.Unlock()
+	m.mut.Unlock()
 
 	_, _, err = m.LoadIgnores("fresh")
 	if err != nil {
@@ -2973,7 +2973,7 @@ func TestConnCloseOnRestart(t *testing.T) {
 	ci := &protocolmocks.ConnectionInfo{}
 	ci.ConnectionIDReturns(srand.String(16))
 	m.AddConnection(protocol.NewConnection(device1, br, nw, testutil.NoopCloser{}, m, ci, protocol.CompressionNever, nil, m.keyGen), protocol.Hello{})
-	m.fmut.RLock()
+	m.mut.RLock()
 	if len(m.closed) != 1 {
 		t.Fatalf("Expected just one conn (len(m.closed) == %v)", len(m.closed))
 	}
@@ -2981,7 +2981,7 @@ func TestConnCloseOnRestart(t *testing.T) {
 	for _, c := range m.closed {
 		closed = c
 	}
-	m.fmut.RUnlock()
+	m.mut.RUnlock()
 
 	waiter, err := w.RemoveDevice(device1)
 	if err != nil {
@@ -3074,12 +3074,12 @@ func TestDevicePause(t *testing.T) {
 	sub := m.evLogger.Subscribe(events.DevicePaused)
 	defer sub.Unsubscribe()
 
-	m.fmut.RLock()
+	m.mut.RLock()
 	var closed chan struct{}
 	for _, c := range m.closed {
 		closed = c
 	}
-	m.fmut.RUnlock()
+	m.mut.RUnlock()
 
 	pauseDevice(t, m.cfg, device1, true)
 
@@ -3754,9 +3754,9 @@ func TestCompletionEmptyGlobal(t *testing.T) {
 	defer wcfgCancel()
 	defer cleanupModelAndRemoveDir(m, fcfg.Filesystem(nil).URI())
 	files := []protocol.FileInfo{{Name: "foo", Version: protocol.Vector{}.Update(myID.Short()), Sequence: 1}}
-	m.fmut.Lock()
+	m.mut.Lock()
 	m.folderFiles[fcfg.ID].Update(protocol.LocalDeviceID, files)
-	m.fmut.Unlock()
+	m.mut.Unlock()
 	files[0].Deleted = true
 	files[0].Version = files[0].Version.Update(device1.Short())
 	must(t, m.IndexUpdate(conn, fcfg.ID, files))

lib/model/requests_test.go

@@ -1287,9 +1287,9 @@ func TestRequestReceiveEncrypted(t *testing.T) {
 
 	files := genFiles(2)
 	files[1].LocalFlags = protocol.FlagLocalReceiveOnly
-	m.fmut.RLock()
+	m.mut.RLock()
 	fset := m.folderFiles[fcfg.ID]
-	m.fmut.RUnlock()
+	m.mut.RUnlock()
 	fset.Update(protocol.LocalDeviceID, files)
 
 	indexChan := make(chan []protocol.FileInfo, 10)

lib/model/testutils_test.go

@@ -295,9 +295,9 @@ func folderIgnoresAlwaysReload(t testing.TB, m *testModel, fcfg config.FolderCon
 	m.removeFolder(fcfg)
 	fset := newFileSet(t, fcfg.ID, m.db)
 	ignores := ignore.New(fcfg.Filesystem(nil), ignore.WithCache(true), ignore.WithChangeDetector(newAlwaysChanged()))
-	m.fmut.Lock()
+	m.mut.Lock()
 	m.addAndStartFolderLockedWithIgnores(fcfg, fset, ignores)
-	m.fmut.Unlock()
+	m.mut.Unlock()
 }
 
 func basicClusterConfig(local, remote protocol.DeviceID, folders ...string) protocol.ClusterConfig {
@@ -319,9 +319,9 @@ func basicClusterConfig(local, remote protocol.DeviceID, folders ...string) prot
 }
 
 func localIndexUpdate(m *testModel, folder string, fs []protocol.FileInfo) {
-	m.fmut.RLock()
+	m.mut.RLock()
 	fset := m.folderFiles[folder]
-	m.fmut.RUnlock()
+	m.mut.RUnlock()
 
 	fset.Update(protocol.LocalDeviceID, fs)
 	seq := fset.Sequence(protocol.LocalDeviceID)

lib/model/util.go

@@ -11,100 +11,12 @@ import (
 	"errors"
 	"fmt"
 	"path/filepath"
-	"strings"
-	"sync"
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
-	"github.com/syncthing/syncthing/lib/events"
 	"github.com/syncthing/syncthing/lib/fs"
-	"github.com/syncthing/syncthing/lib/ur"
 )
 
-type Holdable interface {
-	Holders() string
-}
-
-func newDeadlockDetector(timeout time.Duration, evLogger events.Logger, fatal func(error)) *deadlockDetector {
-	return &deadlockDetector{
-		warnTimeout:  timeout,
-		fatalTimeout: 10 * timeout,
-		lockers:      make(map[string]sync.Locker),
-		evLogger:     evLogger,
-		fatal:        fatal,
-	}
-}
-
-type deadlockDetector struct {
-	warnTimeout, fatalTimeout time.Duration
-	lockers                   map[string]sync.Locker
-	evLogger                  events.Logger
-	fatal                     func(error)
-}
-
-func (d *deadlockDetector) Watch(name string, mut sync.Locker) {
-	d.lockers[name] = mut
-	go func() {
-		for {
-			time.Sleep(d.warnTimeout / 4)
-			done := make(chan struct{}, 1)
-
-			go func() {
-				mut.Lock()
-				_ = 1 // empty critical section
-				mut.Unlock()
-				done <- struct{}{}
-			}()
-
-			d.watchInner(name, done)
-		}
-	}()
-}
-
-func (d *deadlockDetector) watchInner(name string, done chan struct{}) {
-	warn := time.NewTimer(d.warnTimeout)
-	fatal := time.NewTimer(d.fatalTimeout)
-	defer func() {
-		warn.Stop()
-		fatal.Stop()
-	}()
-
-	select {
-	case <-warn.C:
-		failure := ur.FailureDataWithGoroutines(fmt.Sprintf("potential deadlock detected at %s (short timeout)", name))
-		failure.Extra["timeout"] = d.warnTimeout.String()
-		d.evLogger.Log(events.Failure, failure)
-	case <-done:
-		return
-	}
-
-	select {
-	case <-fatal.C:
-		err := fmt.Errorf("potential deadlock detected at %s (long timeout)", name)
-		failure := ur.FailureDataWithGoroutines(err.Error())
-		failure.Extra["timeout"] = d.fatalTimeout.String()
-		others := d.otherHolders()
-		failure.Extra["other-holders"] = others
-		d.evLogger.Log(events.Failure, failure)
-		d.fatal(err)
-		// Give it a minute to shut down gracefully, maybe shutting down
-		// can get out of the deadlock (or it's not really a deadlock).
-		time.Sleep(time.Minute)
-		panic(fmt.Sprintf("%v:\n%v", err, others))
-	case <-done:
-	}
-}
-
-func (d *deadlockDetector) otherHolders() string {
-	var b strings.Builder
-	for otherName, otherMut := range d.lockers {
-		if otherHolder, ok := otherMut.(Holdable); ok {
-			b.WriteString("===" + otherName + "===\n" + otherHolder.Holders() + "\n")
-		}
-	}
-	return b.String()
-}
-
 // inWritableDir calls fn(path), while making sure that the directory
 // containing `path` is writable for the duration of the call.
 func inWritableDir(fn func(string) error, targetFs fs.Filesystem, path string, ignorePerms bool) error {

lib/sync/debug.go

@@ -11,7 +11,6 @@ import (
 	"strconv"
 	"time"
 
-	deadlock "github.com/sasha-s/go-deadlock"
 	"github.com/syncthing/syncthing/lib/logger"
 )
 
@@ -22,8 +21,7 @@ var (
 	// We make an exception in this package and have an actual "if debug { ...
 	// }" variable, as it may be rather performance critical and does
 	// nonstandard things (from a debug logging PoV).
-	debug       = logger.DefaultLogger.ShouldDebug("sync")
-	useDeadlock = false
+	debug = logger.DefaultLogger.ShouldDebug("sync")
 )
 
 func init() {
@@ -31,10 +29,4 @@ func init() {
 		threshold = time.Duration(n) * time.Millisecond
 	}
 	l.Debugf("Enabling lock logging at %v threshold", threshold)
-
-	if n, _ := strconv.Atoi(os.Getenv("STDEADLOCKTIMEOUT")); n > 0 {
-		deadlock.Opts.DeadlockTimeout = time.Duration(n) * time.Second
-		l.Debugf("Enabling lock deadlocking at %v", deadlock.Opts.DeadlockTimeout)
-		useDeadlock = true
-	}
 }

lib/sync/sync.go

@@ -15,8 +15,6 @@ import (
 	"sync"
 	"sync/atomic"
 	"time"
-
-	"github.com/sasha-s/go-deadlock"
 )
 
 var timeNow = time.Now
@@ -39,9 +37,6 @@ type WaitGroup interface {
 }
 
 func NewMutex() Mutex {
-	if useDeadlock {
-		return &deadlock.Mutex{}
-	}
 	if debug {
 		mutex := &loggedMutex{}
 		mutex.holder.Store(holder{})
@@ -51,9 +46,6 @@ func NewMutex() Mutex {
 }
 
 func NewRWMutex() RWMutex {
-	if useDeadlock {
-		return &deadlock.RWMutex{}
-	}
 	if debug {
 		mutex := &loggedRWMutex{
 			readHolders: make(map[int][]holder),

lib/syncthing/syncthing.go

@@ -54,12 +54,11 @@ const (
 )
 
 type Options struct {
-	AuditWriter      io.Writer
-	DeadlockTimeoutS int
-	NoUpgrade        bool
-	ProfilerAddr     string
-	ResetDeltaIdxs   bool
-	Verbose          bool
+	AuditWriter    io.Writer
+	NoUpgrade      bool
+	ProfilerAddr   string
+	ResetDeltaIdxs bool
+	Verbose        bool
 	// null duration means use default value
 	DBRecheckInterval    time.Duration
 	DBIndirectGCInterval time.Duration
@@ -251,12 +250,6 @@ func (a *App) startup() error {
 	keyGen := protocol.NewKeyGenerator()
 	m := model.NewModel(a.cfg, a.myID, a.ll, protectedFiles, a.evLogger, keyGen)
 
-	if a.opts.DeadlockTimeoutS > 0 {
-		m.StartDeadlockDetector(time.Duration(a.opts.DeadlockTimeoutS) * time.Second)
-	} else if !build.IsRelease || build.IsBeta {
-		m.StartDeadlockDetector(20 * time.Minute)
-	}
-
 	a.mainService.Add(m)
 
 	// The TLS configuration is used for both the listening socket and outgoing