Merge pull request #1376 from mtrmac/recursive-remove

Remove lockfile.Locker.RecursiveLock (alternative to #1344)

containers.go

@@ -623,10 +623,6 @@ func (r *containerStore) Lock() {
 	r.lockfile.Lock()
 }
 
-func (r *containerStore) RecursiveLock() {
-	r.lockfile.RecursiveLock()
-}
-
 func (r *containerStore) RLock() {
 	r.lockfile.RLock()
 }

images.go

@@ -804,10 +804,6 @@ func (r *imageStore) Lock() {
 	r.lockfile.Lock()
 }
 
-func (r *imageStore) RecursiveLock() {
-	r.lockfile.RecursiveLock()
-}
-
 func (r *imageStore) RLock() {
 	r.lockfile.RLock()
 }

layers.go

@@ -1920,10 +1920,6 @@ func (r *layerStore) Lock() {
 	r.lockfile.Lock()
 }
 
-func (r *layerStore) RecursiveLock() {
-	r.lockfile.RecursiveLock()
-}
-
 func (r *layerStore) RLock() {
 	r.lockfile.RLock()
 }

pkg/lockfile/lockfile.go

@@ -17,10 +17,6 @@ type Locker interface {
 	// - tried to lock a read-only lock-file
 	Lock()
 
-	// Acquire a writer lock recursively, allowing for recursive acquisitions
-	// within the same process space.
-	RecursiveLock()
-
 	// Unlock the lock.
 	// The default unix implementation panics if:
 	// - unlocking an unlocked lock

pkg/lockfile/lockfile_test.go

@@ -107,45 +107,6 @@ func subLock(l *namedLocker) (io.WriteCloser, io.ReadCloser, error) {
 	return wc, rc, nil
 }
 
-// subRecursiveLockMain is a child process which opens the lock file, closes
-// stdout to indicate that it has acquired the lock, waits for stdin to get
-// closed, and then unlocks the file.
-func subRecursiveLockMain() {
-	if len(os.Args) != 2 {
-		logrus.Fatalf("expected two args, got %d", len(os.Args))
-	}
-	tf, err := GetLockfile(os.Args[1])
-	if err != nil {
-		logrus.Fatalf("error opening lock file %q: %v", os.Args[1], err)
-	}
-	tf.RecursiveLock()
-	os.Stdout.Close()
-	io.Copy(io.Discard, os.Stdin)
-	tf.Unlock()
-}
-
-// subRecursiveLock starts a child process.  If it doesn't return an error, the
-// caller should wait for the first ReadCloser by reading it until it receives
-// an EOF. At that point, the child will have acquired the lock.  It can then
-// signal that the child should release the lock by closing the WriteCloser.
-func subRecursiveLock(l *namedLocker) (io.WriteCloser, io.ReadCloser, error) {
-	cmd := reexec.Command("subRecursiveLock", l.name)
-	wc, err := cmd.StdinPipe()
-	if err != nil {
-		return nil, nil, err
-	}
-	rc, err := cmd.StdoutPipe()
-	if err != nil {
-		return nil, nil, err
-	}
-	go func() {
-		if err = cmd.Run(); err != nil {
-			logrus.Errorf("Running subLock: %v", err)
-		}
-	}()
-	return wc, rc, nil
-}
-
 // subRLockMain is a child process which opens the lock file, closes stdout to
 // indicate that it has acquired the read lock, waits for stdin to get closed,
 // and then unlocks the file.
@@ -188,7 +149,6 @@ func subRLock(l *namedLocker) (io.WriteCloser, io.ReadCloser, error) {
 func init() {
 	reexec.Register("subTouch", subTouchMain)
 	reexec.Register("subRLock", subRLockMain)
-	reexec.Register("subRecursiveLock", subRecursiveLockMain)
 	reexec.Register("subLock", subLockMain)
 }
 
@@ -276,18 +236,6 @@ func TestLockfileWrite(t *testing.T) {
 	l.Unlock()
 }
 
-func TestRecursiveLockfileWrite(t *testing.T) {
-	l, err := getTempLockfile()
-	require.Nil(t, err, "error getting temporary lock file")
-	defer os.Remove(l.name)
-
-	l.RecursiveLock()
-	assert.True(t, l.Locked(), "Locked() said we didn't have a write lock")
-	l.RecursiveLock()
-	l.Unlock()
-	l.Unlock()
-}
-
 func TestROLockfileWrite(t *testing.T) {
 	l, err := getTempROLockfile()
 	require.Nil(t, err, "error getting temporary lock file")
@@ -403,44 +351,6 @@ func TestLockfileReadConcurrent(t *testing.T) {
 	}
 }
 
-func TestLockfileRecursiveWrite(t *testing.T) {
-	// NOTE: given we're in the same process space, it's effectively the same as
-	// reader lock.
-
-	l, err := getTempLockfile()
-	require.Nil(t, err, "error getting temporary lock file")
-	defer os.Remove(l.name)
-
-	// the test below is inspired by the stdlib's rwmutex tests
-	numReaders := 1000
-	locked := make(chan bool)
-	unlocked := make(chan bool)
-	done := make(chan bool)
-
-	for i := 0; i < numReaders; i++ {
-		go func() {
-			l.RecursiveLock()
-			locked <- true
-			<-unlocked
-			l.Unlock()
-			done <- true
-		}()
-	}
-
-	// Wait for all parallel locks to succeed
-	for i := 0; i < numReaders; i++ {
-		<-locked
-	}
-	// Instruct all parallel locks to unlock
-	for i := 0; i < numReaders; i++ {
-		unlocked <- true
-	}
-	// Wait for all parallel locks to be unlocked
-	for i := 0; i < numReaders; i++ {
-		<-done
-	}
-}
-
 func TestLockfileMixedConcurrent(t *testing.T) {
 	l, err := getTempLockfile()
 	require.Nil(t, err, "error getting temporary lock file")
@@ -495,63 +405,6 @@ func TestLockfileMixedConcurrent(t *testing.T) {
 	}
 }
 
-func TestLockfileMixedConcurrentRecursiveWriters(t *testing.T) {
-	// It's effectively the same tests as with mixed readers & writers but calling
-	// RecursiveLocks() instead.
-
-	l, err := getTempLockfile()
-	require.Nil(t, err, "error getting temporary lock file")
-	defer os.Remove(l.name)
-
-	counter := int32(0)
-	diff := int32(10000)
-	numIterations := 10
-	numReaders := 100
-	numWriters := 50
-
-	done := make(chan bool)
-
-	// A writer always adds `diff` to the counter. Hence, `diff` is the
-	// only valid value in the critical section.
-	writer := func(c *int32) {
-		for i := 0; i < numIterations; i++ {
-			l.Lock()
-			tmp := atomic.AddInt32(c, diff)
-			assert.True(t, tmp == diff, "counter should be %d but instead is %d", diff, tmp)
-			time.Sleep(100 * time.Millisecond)
-			atomic.AddInt32(c, diff*(-1))
-			l.Unlock()
-		}
-		done <- true
-	}
-
-	// A reader always adds `1` to the counter. Hence,
-	// [1,`numReaders*numIterations`] are valid values.
-	reader := func(c *int32) {
-		for i := 0; i < numIterations; i++ {
-			l.RecursiveLock()
-			tmp := atomic.AddInt32(c, 1)
-			assert.True(t, tmp >= 1 && tmp < diff)
-			time.Sleep(100 * time.Millisecond)
-			atomic.AddInt32(c, -1)
-			l.Unlock()
-		}
-		done <- true
-	}
-
-	for i := 0; i < numReaders; i++ {
-		go reader(&counter)
-		// schedule a writer every 2nd iteration
-		if i%2 == 1 {
-			go writer(&counter)
-		}
-	}
-
-	for i := 0; i < numReaders+numWriters; i++ {
-		<-done
-	}
-}
-
 func TestLockfileMultiprocessRead(t *testing.T) {
 	l, err := getTempLockfile()
 	require.Nil(t, err, "error getting temporary lock file")
@@ -638,49 +491,6 @@ func TestLockfileMultiprocessWrite(t *testing.T) {
 	assert.True(t, whighest == 1, "expected to have no more than one writer lock active at a time, had %d", whighest)
 }
 
-func TestLockfileMultiprocessRecursiveWrite(t *testing.T) {
-	l, err := getTempLockfile()
-	require.Nil(t, err, "error getting temporary lock file")
-	defer os.Remove(l.name)
-	var wg sync.WaitGroup
-	var wcounter, whighest int64
-	var highestMutex sync.Mutex
-	subs := make([]struct {
-		stdin  io.WriteCloser
-		stdout io.ReadCloser
-	}, 10)
-	for i := range subs {
-		stdin, stdout, err := subRecursiveLock(l)
-		require.Nil(t, err, "error starting subprocess %d to take a write lock", i+1)
-		subs[i].stdin = stdin
-		subs[i].stdout = stdout
-	}
-	for i := range subs {
-		wg.Add(1)
-		go func(i int) {
-			io.Copy(io.Discard, subs[i].stdout)
-			if testing.Verbose() {
-				t.Logf("\tchild %4d acquired the recursive write lock\n", i+1)
-			}
-			workingWcounter := atomic.AddInt64(&wcounter, 1)
-			highestMutex.Lock()
-			if workingWcounter > whighest {
-				whighest = workingWcounter
-			}
-			highestMutex.Unlock()
-			time.Sleep(1 * time.Second)
-			atomic.AddInt64(&wcounter, -1)
-			if testing.Verbose() {
-				t.Logf("\ttelling child %4d to release the recursive write lock\n", i+1)
-			}
-			subs[i].stdin.Close()
-			wg.Done()
-		}(i)
-	}
-	wg.Wait()
-	assert.True(t, whighest == 1, "expected to have no more than one writer lock active at a time, had %d", whighest)
-}
-
 func TestLockfileMultiprocessMixed(t *testing.T) {
 	l, err := getTempLockfile()
 	require.Nil(t, err, "error getting temporary lock file")

pkg/lockfile/lockfile_unix.go

@@ -30,7 +30,6 @@ type lockfile struct {
 	locktype   int16
 	locked     bool
 	ro         bool
-	recursive  bool
 }
 
 const lastWriterIDSize = 64    // This must be the same as len(stringid.GenerateRandomID)
@@ -131,7 +130,7 @@ func createLockerForPath(path string, ro bool) (Locker, error) {
 
 // lock locks the lockfile via FCTNL(2) based on the specified type and
 // command.
-func (l *lockfile) lock(lType int16, recursive bool) {
+func (l *lockfile) lock(lType int16) {
 	lk := unix.Flock_t{
 		Type:   lType,
 		Whence: int16(os.SEEK_SET),
@@ -142,13 +141,7 @@ func (l *lockfile) lock(lType int16, recursive bool) {
 	case unix.F_RDLCK:
 		l.rwMutex.RLock()
 	case unix.F_WRLCK:
-		if recursive {
-			// NOTE: that's okay as recursive is only set in RecursiveLock(), so
-			// there's no need to protect against hypothetical RDLCK cases.
-			l.rwMutex.RLock()
-		} else {
-			l.rwMutex.Lock()
-		}
+		l.rwMutex.Lock()
 	default:
 		panic(fmt.Sprintf("attempted to acquire a file lock of unrecognized type %d", lType))
 	}
@@ -171,7 +164,6 @@ func (l *lockfile) lock(lType int16, recursive bool) {
 	}
 	l.locktype = lType
 	l.locked = true
-	l.recursive = recursive
 	l.counter++
 }
 
@@ -180,24 +172,13 @@ func (l *lockfile) Lock() {
 	if l.ro {
 		panic("can't take write lock on read-only lock file")
 	} else {
-		l.lock(unix.F_WRLCK, false)
-	}
-}
-
-// RecursiveLock locks the lockfile as a writer but allows for recursive
-// acquisitions within the same process space.  Note that RLock() will be called
-// if it's a lockTypReader lock.
-func (l *lockfile) RecursiveLock() {
-	if l.ro {
-		l.RLock()
-	} else {
-		l.lock(unix.F_WRLCK, true)
+		l.lock(unix.F_WRLCK)
 	}
 }
 
 // LockRead locks the lockfile as a reader.
 func (l *lockfile) RLock() {
-	l.lock(unix.F_RDLCK, false)
+	l.lock(unix.F_RDLCK)
 }
 
 // Unlock unlocks the lockfile.
@@ -224,7 +205,7 @@ func (l *lockfile) Unlock() {
 		// file lock.
 		unix.Close(int(l.fd))
 	}
-	if l.locktype == unix.F_RDLCK || l.recursive {
+	if l.locktype == unix.F_RDLCK {
 		l.rwMutex.RUnlock()
 	} else {
 		l.rwMutex.Unlock()

pkg/lockfile/lockfile_windows.go

@@ -1,3 +1,4 @@
+//go:build windows
 // +build windows
 
 package lockfile
@@ -36,12 +37,6 @@ func (l *lockfile) Lock() {
 	l.locked = true
 }
 
-func (l *lockfile) RecursiveLock() {
-	// We don't support Windows but a recursive writer-lock in one process-space
-	// is really a writer lock, so just panic.
-	panic("not supported")
-}
-
 func (l *lockfile) RLock() {
 	l.mu.Lock()
 	l.locked = true