From 2a3d59997fb9731bae6f0dd3b23d4b17455fed29 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Fri, 28 Jan 2022 19:11:56 -0800 Subject: [PATCH] Enable reference count leak checking for lisafs. Also add DoRepeatedLeakCheck() to refsvfs2 package. Updates #5466 PiperOrigin-RevId: 425004987 --- pkg/lisafs/connection_test.go | 1 + pkg/lisafs/server.go | 5 ++++ pkg/lisafs/testsuite/BUILD | 2 ++ pkg/lisafs/testsuite/testsuite.go | 18 +++++++++---- pkg/refsvfs2/refs_map.go | 45 +++++++++++++++++++------------ runsc/cmd/gofer.go | 1 + 6 files changed, 50 insertions(+), 22 deletions(-) diff --git a/pkg/lisafs/connection_test.go b/pkg/lisafs/connection_test.go index 1d1e430de..616f40ef2 100644 --- a/pkg/lisafs/connection_test.go +++ b/pkg/lisafs/connection_test.go @@ -104,6 +104,7 @@ func runServerClient(t testing.TB, clientFn func(c *lisafs.Client)) { c.Close() // This should trigger client and server shutdown. ts.Wait() + ts.Server.Destroy() } // TestStartUp tests that the server and client can be started up correctly. diff --git a/pkg/lisafs/server.go b/pkg/lisafs/server.go index 681532960..895ad1742 100644 --- a/pkg/lisafs/server.go +++ b/pkg/lisafs/server.go @@ -103,6 +103,11 @@ func (s *Server) Wait() { s.connWg.Wait() } +// Destroy releases resources being used by this server. +func (s *Server) Destroy() { + s.root.DecRef(nil) +} + // ServerImpl contains the implementation details for a Server. // Implementations of ServerImpl should contain their associated Server by // value as their first field. diff --git a/pkg/lisafs/testsuite/BUILD b/pkg/lisafs/testsuite/BUILD index b4a542b3a..9e795fd02 100644 --- a/pkg/lisafs/testsuite/BUILD +++ b/pkg/lisafs/testsuite/BUILD @@ -13,6 +13,8 @@ go_library( "//pkg/abi/linux", "//pkg/context", "//pkg/lisafs", + "//pkg/refs", + "//pkg/refsvfs2", "//pkg/unet", "@com_github_syndtr_gocapability//capability:go_default_library", "@org_golang_x_sys//unix:go_default_library", diff --git a/pkg/lisafs/testsuite/testsuite.go b/pkg/lisafs/testsuite/testsuite.go index 936d70f77..8cebca85f 100644 --- a/pkg/lisafs/testsuite/testsuite.go +++ b/pkg/lisafs/testsuite/testsuite.go @@ -30,6 +30,8 @@ import ( "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/lisafs" + "gvisor.dev/gvisor/pkg/refs" + "gvisor.dev/gvisor/pkg/refsvfs2" "gvisor.dev/gvisor/pkg/unet" ) @@ -49,10 +51,9 @@ type Tester interface { // RunAllLocalFSTests runs all local FS tests as subtests. func RunAllLocalFSTests(t *testing.T, tester Tester) { + refs.SetLeakMode(refs.LeaksPanic) for name, testFn := range localFSTests { - t.Run(name, func(t *testing.T) { - runServerClient(t, tester, testFn) - }) + runServerClient(t, tester, name, testFn) } } @@ -74,7 +75,7 @@ var localFSTests map[string]testFunc = map[string]testFunc{ "Getdents": testGetdents, } -func runServerClient(t *testing.T, tester Tester, testFn testFunc) { +func runServerClient(t *testing.T, tester Tester, testName string, testFn testFunc) { mountPath, err := ioutil.TempDir(os.Getenv("TEST_TMPDIR"), "") if err != nil { t.Fatalf("creation of temporary mountpoint failed: %v", err) @@ -109,9 +110,16 @@ func runServerClient(t *testing.T, tester Tester, testFn testFunc) { rootFile := c.NewFD(root.ControlFD) ctx := context.Background() - testFn(ctx, t, tester, rootFile) + t.Run(testName, func(t *testing.T) { + testFn(ctx, t, tester, rootFile) + }) closeFD(ctx, t, rootFile) + // Release server resources and check for leaks. Note that leak check must + // happen before c.Close() because server cleans up resources on shutdown. + server.Destroy() + refsvfs2.DoRepeatedLeakCheck() + c.Close() // This should trigger client and server shutdown. server.Wait() } diff --git a/pkg/refsvfs2/refs_map.go b/pkg/refsvfs2/refs_map.go index a1146a344..8bc3ef377 100644 --- a/pkg/refsvfs2/refs_map.go +++ b/pkg/refsvfs2/refs_map.go @@ -124,24 +124,35 @@ func logEvent(obj CheckedObject, msg string) { var checkOnce sync.Once // DoLeakCheck iterates through the live object map and logs a message for each -// object. It is called once no reference-counted objects should be reachable -// anymore, at which point anything left in the map is considered a leak. +// object. It should be called when no reference-counted objects are reachable +// anymore, at which point anything left in the map is considered a leak. On +// multiple calls, only the first call will perform the leak check. func DoLeakCheck() { if leakCheckEnabled() { - checkOnce.Do(func() { - liveObjectsMu.Lock() - defer liveObjectsMu.Unlock() - leaked := len(liveObjects) - if leaked > 0 { - msg := fmt.Sprintf("Leak checking detected %d leaked objects:\n", leaked) - for obj := range liveObjects { - msg += obj.LeakMessage() + "\n" - } - if leakCheckPanicEnabled() { - panic(msg) - } - log.Warningf(msg) - } - }) + checkOnce.Do(doLeakCheck) + } +} + +// DoRepeatedLeakCheck is the same as DoLeakCheck except that it can be called +// multiple times by the caller to incrementally perform leak checking. +func DoRepeatedLeakCheck() { + if leakCheckEnabled() { + doLeakCheck() + } +} + +func doLeakCheck() { + liveObjectsMu.Lock() + defer liveObjectsMu.Unlock() + leaked := len(liveObjects) + if leaked > 0 { + msg := fmt.Sprintf("Leak checking detected %d leaked objects:\n", leaked) + for obj := range liveObjects { + msg += obj.LeakMessage() + "\n" + } + if leakCheckPanicEnabled() { + panic(msg) + } + log.Warningf(msg) } } diff --git a/runsc/cmd/gofer.go b/runsc/cmd/gofer.go index 7c22439c7..6e5d00369 100644 --- a/runsc/cmd/gofer.go +++ b/runsc/cmd/gofer.go @@ -245,6 +245,7 @@ func (g *Gofer) serveLisafs(spec *specs.Spec, conf *config.Config, root string) server.StartConnection(conn) } server.Wait() + server.Destroy() log.Infof("All lisafs servers exited.") return subcommands.ExitSuccess }