From d13bc9efcacdbdd00654b25f6cc9403a52060b69 Mon Sep 17 00:00:00 2001
From: Ayush Ranjan <ayushranjan@google.com>
Date: Tue, 15 Feb 2022 19:42:03 -0800
Subject: [PATCH] Update lisafs to use defer to Unlock locks.

There can be situations where server implementations choose to panic. lisafs
package should be prepared to recover from such situations. Currently panicked
goroutines recover and return EREMOTEIO. But there are code paths where locks
will not be unlocked on panic. So defer the calls to unlock so that locks are
unlocked correctly on the path from panic() to recover().

This should not have any performance ramifications because these are all open-
coded defers (which are not heap allocated).

PiperOrigin-RevId: 428936943
---
 pkg/lisafs/connection.go | 2 +-
 pkg/lisafs/fd.go         | 2 +-
 pkg/lisafs/node.go       | 8 ++++----
 pkg/lisafs/server.go     | 5 ++---
 4 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/pkg/lisafs/connection.go b/pkg/lisafs/connection.go
index 38d769e7d..31e6bd8c6 100644
--- a/pkg/lisafs/connection.go
+++ b/pkg/lisafs/connection.go
@@ -245,11 +245,11 @@ func (c *Connection) close() {
 
 	// Cleanup all FDs.
 	c.fdsMu.Lock()
+	defer c.fdsMu.Unlock()
 	for fdid := range c.fds {
 		fd := c.stopTrackingFD(fdid)
 		fd.DecRef(nil) // Drop the ref held by c.
 	}
-	c.fdsMu.Unlock()
 }
 
 // Postcondition: The caller gains a ref on the FD on success.
diff --git a/pkg/lisafs/fd.go b/pkg/lisafs/fd.go
index 87ef76c04..b2e254bc0 100644
--- a/pkg/lisafs/fd.go
+++ b/pkg/lisafs/fd.go
@@ -90,8 +90,8 @@ var _ genericFD = (*ControlFD)(nil)
 func (fd *ControlFD) DecRef(context.Context) {
 	fd.controlFDRefs.DecRef(func() {
 		fd.conn.server.renameMu.RLock()
+		defer fd.conn.server.renameMu.RUnlock()
 		fd.destroyLocked()
-		fd.conn.server.renameMu.RUnlock()
 	})
 }
 
diff --git a/pkg/lisafs/node.go b/pkg/lisafs/node.go
index 79e63e508..662bd8fff 100644
--- a/pkg/lisafs/node.go
+++ b/pkg/lisafs/node.go
@@ -161,8 +161,8 @@ func (n *Node) LookupChildLocked(name string) *Node {
 // WithChildrenMu executes fn with n.childrenMu locked.
 func (n *Node) WithChildrenMu(fn func()) {
 	n.childrenMu.Lock()
+	defer n.childrenMu.Unlock()
 	fn()
-	n.childrenMu.Unlock()
 }
 
 // FilePath returns the absolute path of the backing file. This is an expensive
@@ -188,22 +188,22 @@ func (n *Node) isDeleted() bool {
 
 func (n *Node) removeFD(fd *ControlFD) {
 	n.controlFDsMu.Lock()
+	defer n.controlFDsMu.Unlock()
 	n.controlFDs.Remove(fd)
-	n.controlFDsMu.Unlock()
 }
 
 func (n *Node) insertFD(fd *ControlFD) {
 	n.controlFDsMu.Lock()
+	defer n.controlFDsMu.Unlock()
 	n.controlFDs.PushBack(fd)
-	n.controlFDsMu.Unlock()
 }
 
 func (n *Node) forEachFD(fn func(*ControlFD)) {
 	n.controlFDsMu.Lock()
+	defer n.controlFDsMu.Unlock()
 	for fd := n.controlFDs.Front(); fd != nil; fd = fd.Next() {
 		fn(fd)
 	}
-	n.controlFDsMu.Unlock()
 }
 
 // removeChildLocked removes child with given name from n and returns the
diff --git a/pkg/lisafs/server.go b/pkg/lisafs/server.go
index 895ad1742..d419f4228 100644
--- a/pkg/lisafs/server.go
+++ b/pkg/lisafs/server.go
@@ -84,9 +84,8 @@ func (s *Server) SetHandlers(handlers []RPCHandler) {
 // reading. This ensures that no rename operations occur concurrently.
 func (s *Server) withRenameReadLock(fn func() error) error {
 	s.renameMu.RLock()
-	err := fn()
-	s.renameMu.RUnlock()
-	return err
+	defer s.renameMu.RUnlock()
+	return fn()
 }
 
 // StartConnection starts the connection on a separate goroutine and tracks it.