PR feedback: removing some duplication around use of TryLock to assert that mutexes are locked and adding some extra comments.

go/libraries/doltcore/sqle/database_provider.go

@@ -38,6 +38,7 @@ import (
 	"github.com/dolthub/dolt/go/libraries/doltcore/sqlserver"
 	"github.com/dolthub/dolt/go/libraries/doltcore/table/editor"
 	"github.com/dolthub/dolt/go/libraries/utils/filesys"
+	"github.com/dolthub/dolt/go/libraries/utils/lockutil"
 	"github.com/dolthub/dolt/go/store/datas"
 	"github.com/dolthub/dolt/go/store/types"
 )
@@ -658,11 +659,8 @@ func (p *DoltDatabaseProvider) PurgeDroppedDatabases(ctx *sql.Context) error {
 // function is responsible for instantiating the new Database instance and updating the tracking metadata
 // in this provider. If any problems are encountered while registering the new database, an error is returned.
 func (p *DoltDatabaseProvider) registerNewDatabase(ctx *sql.Context, name string, newEnv *env.DoltEnv) (err error) {
-	// This method MUST be called with the provider's mutex locked. TryLock allows us to validate that the
-	// mutex is locked (without actually locking it and causing a deadlock) and to error out if we detect
-	// that the mutex is NOT locked.
-	if p.mu.TryLock() {
-		defer p.mu.Unlock()
+	// This method MUST be called with the provider's mutex locked
+	if err = lockutil.AssertRWMutexIsLocked(p.mu); err != nil {
 		return fmt.Errorf("unable to register new database without database provider mutex being locked")
 	}
 

go/libraries/utils/filesys/inmemfs.go

@@ -28,6 +28,7 @@ import (
 	"time"
 
 	"github.com/dolthub/dolt/go/libraries/utils/iohelp"
+	"github.com/dolthub/dolt/go/libraries/utils/lockutil"
 	"github.com/dolthub/dolt/go/libraries/utils/osutil"
 )
 
@@ -518,10 +519,8 @@ func (fs *InMemFS) MoveDir(srcPath, destPath string) error {
 }
 
 func (fs *InMemFS) moveDirHelper(dir *memDir, destPath string) error {
-	// All calls to moveDirHelper should happen with the filesystem's read-write mutex locked;
-	// if we detect the mutex isn't locked, then return an error.
-	if fs.rwLock.TryLock() {
-		defer fs.rwLock.Unlock()
+	// All calls to moveDirHelper MUST happen with the filesystem's read-write mutex locked
+	if err := lockutil.AssertRWMutexIsLocked(fs.rwLock); err != nil {
 		return fmt.Errorf("moveDirHelper called without first aquiring filesystem read-write lock")
 	}
 
@@ -596,10 +595,8 @@ func (fs *InMemFS) MoveFile(srcPath, destPath string) error {
 }
 
 func (fs *InMemFS) moveFileHelper(obj *memFile, destPath string) error {
-	// All calls to moveFileHelper should happen with the filesystem's read-write mutex locked;
-	// if we detect the mutex isn't locked, then return an error.
-	if fs.rwLock.TryLock() {
-		defer fs.rwLock.Unlock()
+	// All calls to moveFileHelper MUST happen with the filesystem's read-write mutex locked
+	if err := lockutil.AssertRWMutexIsLocked(fs.rwLock); err != nil {
 		return fmt.Errorf("moveFileHelper called without first aquiring filesystem read-write lock")
 	}
 

go/libraries/utils/lockutil/lockutil.go

@@ -0,0 +1,38 @@
+// Copyright 2023 Dolthub, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package lockutil
+
+import (
+	"errors"
+	"sync"
+)
+
+// ErrMutexNotLocked is the error returned by AssertRWMutexIsLocked if the specified mutex was not locked.
+var ErrMutexNotLocked = errors.New("mutex is not locked")
+
+// AssertRWMutexIsLocked checks if |mu| is locked (without deadlocking if the mutex is locked) and returns nil
+// the mutex is locked. If the mutex is NOT locked, the ErrMutexNotLocked error is returned.
+func AssertRWMutexIsLocked(mu *sync.RWMutex) error {
+	// TryLock allows us to validate that the mutex is locked (without actually locking it and causing a
+	// deadlock) and to return an error if we detect that the mutex is NOT locked.
+	if mu.TryLock() {
+		// If TryLock returns true, that means it was able to successfully acquire the lock on the mutex, which
+		// means the caller did NOT have the lock when they called this function, so we return an error, and we
+		// also need to release the lock on the mutex that we grabbed while testing whether the mutex was locked.
+		defer mu.Unlock()
+		return ErrMutexNotLocked
+	}
+	return nil
+}