17464: Add DebugLocksPanicMode and fix some missing locks.

refs #17464

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

diff --git a/sdk/go/arvados/fs_base.go b/sdk/go/arvados/fs_base.go
index 2478641df5478639c92ff2b4d0f57d847165eee7..65c207162b0f8590f463097faa3b7de25043f5fc 100644 (file)
--- 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 0233826a7281e9aa95f5dbb9f74e93ddb1bfd473..22e2b31d57e08d6c5dc813017c62b950f61aac01 100644 (file)
--- a/sdk/go/arvados/fs_collection.go
+++ b/sdk/go/arvados/fs_collection.go
@@ -1167,9 +1167,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
                                }
@@ -1181,6 +1184,7 @@ func (dn *dirnode) createFileAndParents(path string) (fn *filenode, err error) {
                                return child, nil
                        }
                })
+               locked.Unlock()
                if err != nil {
                        return
                }
@@ -1191,10 +1195,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 900893aa36420e7c9d2008fff31b36d4bd03e0bf..5225df59ee58ec4c2017054f59ca68ee7936e41e 100644 (file)
--- 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 b1c627f89c9e43c198dbd7a2a59f059c3cb01372..a41a9f4da992f34febd1312d03d25b3995344658 100644 (file)
--- 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 446d591bfd715224651c1d9667e0c451e81f664e..8715ab24f35c0312fcca8152dc464ab60aa582af 100644 (file)
--- a/services/keep-web/handler_test.go
+++ b/services/keep-web/handler_test.go
@@ -32,6 +32,10 @@ import (
 
 var _ = check.Suite(&UnitSuite{})
 
+func init() {
+       arvados.DebugLocksPanicMode = true
+}
+
 type UnitSuite struct {
        Config *arvados.Config
 }