[ARVADOS] created: 2.1.0-896-g87033e4c4

[Git user]

        at  87033e4c4aae0fd4366d1d807e87fb4bb7e7af9d (commit)


commit 87033e4c4aae0fd4366d1d807e87fb4bb7e7af9d
Author: Tom Clegg <tom at curii.com>
Date:   Thu Jun 17 15:18:25 2021 -0400

    17464: Add DebugLocksPanicMode and fix some missing locks.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/sdk/go/arvados/fs_base.go b/sdk/go/arvados/fs_base.go
index 2478641df..65c207162 100644
--- a/sdk/go/arvados/fs_base.go
+++ b/sdk/go/arvados/fs_base.go
@@ -29,12 +29,29 @@ var (
 	ErrIsDirectory       = errors.New("cannot rename file to overwrite existing directory")
 	ErrNotADirectory     = errors.New("not a directory")
 	ErrPermission        = os.ErrPermission
+	DebugLocksPanicMode  = false
 )
 
 type syncer interface {
 	Sync() error
 }
 
+func debugPanicIfNotLocked(l sync.Locker) {
+	if !DebugLocksPanicMode {
+		return
+	}
+	race := false
+	go func() {
+		l.Lock()
+		race = true
+		l.Unlock()
+	}()
+	time.Sleep(10)
+	if race {
+		panic("bug: caller-must-have-lock func called, but nobody has lock")
+	}
+}
+
 // A File is an *os.File-like interface for reading and writing files
 // in a FileSystem.
 type File interface {
@@ -271,6 +288,7 @@ func (n *treenode) IsDir() bool {
 }
 
 func (n *treenode) Child(name string, replace func(inode) (inode, error)) (child inode, err error) {
+	debugPanicIfNotLocked(n)
 	child = n.inodes[name]
 	if name == "" || name == "." || name == ".." {
 		err = ErrInvalidArgument
@@ -333,6 +351,7 @@ func (n *treenode) Sync() error {
 func (n *treenode) MemorySize() (size int64) {
 	n.RLock()
 	defer n.RUnlock()
+	debugPanicIfNotLocked(n)
 	for _, inode := range n.inodes {
 		size += inode.MemorySize()
 	}
diff --git a/sdk/go/arvados/fs_collection.go b/sdk/go/arvados/fs_collection.go
index 4dbd4b774..b743ab368 100644
--- a/sdk/go/arvados/fs_collection.go
+++ b/sdk/go/arvados/fs_collection.go
@@ -1168,9 +1168,12 @@ func (dn *dirnode) createFileAndParents(path string) (fn *filenode, err error) {
 			node = node.Parent()
 			continue
 		}
+		modtime := node.Parent().FileInfo().ModTime()
+		node.Lock()
+		locked := node
 		node, err = node.Child(name, func(child inode) (inode, error) {
 			if child == nil {
-				child, err := node.FS().newNode(name, 0755|os.ModeDir, node.Parent().FileInfo().ModTime())
+				child, err := node.FS().newNode(name, 0755|os.ModeDir, modtime)
 				if err != nil {
 					return nil, err
 				}
@@ -1182,6 +1185,7 @@ func (dn *dirnode) createFileAndParents(path string) (fn *filenode, err error) {
 				return child, nil
 			}
 		})
+		locked.Unlock()
 		if err != nil {
 			return
 		}
@@ -1192,10 +1196,13 @@ func (dn *dirnode) createFileAndParents(path string) (fn *filenode, err error) {
 		err = fmt.Errorf("invalid file part %q in path %q", basename, path)
 		return
 	}
+	modtime := node.FileInfo().ModTime()
+	node.Lock()
+	defer node.Unlock()
 	_, err = node.Child(basename, func(child inode) (inode, error) {
 		switch child := child.(type) {
 		case nil:
-			child, err = node.FS().newNode(basename, 0755, node.FileInfo().ModTime())
+			child, err = node.FS().newNode(basename, 0755, modtime)
 			if err != nil {
 				return nil, err
 			}
diff --git a/sdk/go/arvados/fs_site.go b/sdk/go/arvados/fs_site.go
index 900893aa3..5225df59e 100644
--- a/sdk/go/arvados/fs_site.go
+++ b/sdk/go/arvados/fs_site.go
@@ -54,6 +54,8 @@ func (c *Client) CustomFileSystem(kc keepClient) CustomFileSystem {
 }
 
 func (fs *customFileSystem) MountByID(mount string) {
+	fs.root.treenode.Lock()
+	defer fs.root.treenode.Unlock()
 	fs.root.treenode.Child(mount, func(inode) (inode, error) {
 		return &vdirnode{
 			treenode: treenode{
@@ -72,12 +74,16 @@ func (fs *customFileSystem) MountByID(mount string) {
 }
 
 func (fs *customFileSystem) MountProject(mount, uuid string) {
+	fs.root.treenode.Lock()
+	defer fs.root.treenode.Unlock()
 	fs.root.treenode.Child(mount, func(inode) (inode, error) {
 		return fs.newProjectNode(fs.root, mount, uuid), nil
 	})
 }
 
 func (fs *customFileSystem) MountUsers(mount string) {
+	fs.root.treenode.Lock()
+	defer fs.root.treenode.Unlock()
 	fs.root.treenode.Child(mount, func(inode) (inode, error) {
 		return &lookupnode{
 			stale:   fs.Stale,
diff --git a/sdk/go/arvados/fs_site_test.go b/sdk/go/arvados/fs_site_test.go
index b1c627f89..a41a9f4da 100644
--- a/sdk/go/arvados/fs_site_test.go
+++ b/sdk/go/arvados/fs_site_test.go
@@ -32,6 +32,15 @@ const (
 
 var _ = check.Suite(&SiteFSSuite{})
 
+func init() {
+	// Enable DebugLocksPanicMode sometimes. Don't enable it all
+	// the time, though -- it adds many calls to time.Sleep(),
+	// which could hide different bugs.
+	if time.Now().Seconds()&1 == 0 {
+		DebugLocksPanicMode = true
+	}
+}
+
 type SiteFSSuite struct {
 	client *Client
 	fs     CustomFileSystem
diff --git a/services/keep-web/handler_test.go b/services/keep-web/handler_test.go
index 31724dd5d..726c6220e 100644
--- a/services/keep-web/handler_test.go
+++ b/services/keep-web/handler_test.go
@@ -33,6 +33,10 @@ import (
 
 var _ = check.Suite(&UnitSuite{})
 
+func init() {
+	arvados.DebugLocksPanicMode = true
+}
+
 type UnitSuite struct {
 	Config *arvados.Config
 }

-----------------------------------------------------------------------


hooks/post-receive
--