From 2c9b078fb471e0ef64e3cb50d04bd1269465aeb2 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Mon, 29 Nov 2021 14:17:57 -0500 Subject: [PATCH] internal/memoize: record the caller of Destroy The hypothesis for golang/go#48774 is that the generation is being destroyed by a call to (*View).shutdown. This change adds a bit of logging to confirm that hypothesis. For golang/go#48774 Change-Id: I34be2e16a0dcab4cea7e9b704b56f4cf0abb0c71 Reviewed-on: https://go-review.googlesource.com/c/tools/+/367674 Trust: Bryan C. Mills Run-TryBot: Bryan C. Mills gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Robert Findley --- internal/lsp/cache/view.go | 4 ++-- internal/memoize/memoize.go | 20 ++++++++++++++------ internal/memoize/memoize_test.go | 8 ++++---- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index b9695732e6b..91c910828aa 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -505,7 +505,7 @@ func (v *View) shutdown(ctx context.Context) { } v.mu.Unlock() v.snapshotMu.Lock() - go v.snapshot.generation.Destroy() + go v.snapshot.generation.Destroy("View.shutdown") v.snapshotMu.Unlock() v.importsState.destroy() } @@ -694,7 +694,7 @@ func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]*file event.Error(ctx, "copying workspace dir", err) } } - go oldSnapshot.generation.Destroy() + go oldSnapshot.generation.Destroy("View.invalidateContent") return v.snapshot, v.snapshot.generation.Acquire(ctx) } diff --git a/internal/memoize/memoize.go b/internal/memoize/memoize.go index d4b87731590..6127fc3e20d 100644 --- a/internal/memoize/memoize.go +++ b/internal/memoize/memoize.go @@ -62,6 +62,8 @@ type Generation struct { destroyed uint32 store *Store name string + // destroyedBy describes the caller that togged destroyed from 0 to 1. + destroyedBy string // wg tracks the reference count of this generation. wg sync.WaitGroup } @@ -69,10 +71,16 @@ type Generation struct { // Destroy waits for all operations referencing g to complete, then removes // all references to g from cache entries. Cache entries that no longer // reference any non-destroyed generation are removed. Destroy must be called -// exactly once for each generation. -func (g *Generation) Destroy() { +// exactly once for each generation, and destroyedBy describes the caller. +func (g *Generation) Destroy(destroyedBy string) { g.wg.Wait() - atomic.StoreUint32(&g.destroyed, 1) + + prevDestroyedBy := g.destroyedBy + g.destroyedBy = destroyedBy + if ok := atomic.CompareAndSwapUint32(&g.destroyed, 0, 1); !ok { + panic("Destroy on generation " + g.name + " already destroyed by " + prevDestroyedBy) + } + g.store.mu.Lock() defer g.store.mu.Unlock() for k, e := range g.store.handles { @@ -100,7 +108,7 @@ func (g *Generation) Acquire(ctx context.Context) func() { return func() {} } if destroyed != 0 { - panic("acquire on destroyed generation " + g.name) + panic("acquire on generation " + g.name + " destroyed by " + g.destroyedBy) } g.wg.Add(1) return g.wg.Done @@ -175,7 +183,7 @@ func (g *Generation) Bind(key interface{}, function Function, cleanup func(inter panic("the function passed to bind must not be nil") } if atomic.LoadUint32(&g.destroyed) != 0 { - panic("operation on destroyed generation " + g.name) + panic("operation on generation " + g.name + " destroyed by " + g.destroyedBy) } g.store.mu.Lock() defer g.store.mu.Unlock() @@ -233,7 +241,7 @@ func (s *Store) DebugOnlyIterate(f func(k, v interface{})) { func (g *Generation) Inherit(hs ...*Handle) { for _, h := range hs { if atomic.LoadUint32(&g.destroyed) != 0 { - panic("inherit on destroyed generation " + g.name) + panic("inherit on generation " + g.name + " destroyed by " + g.destroyedBy) } h.mu.Lock() diff --git a/internal/memoize/memoize_test.go b/internal/memoize/memoize_test.go index 41f20d0bce2..f05966b4614 100644 --- a/internal/memoize/memoize_test.go +++ b/internal/memoize/memoize_test.go @@ -60,11 +60,11 @@ func TestGenerations(t *testing.T) { expectGet(t, h2, g2, "res") // With g1 destroyed, g2 should still work. - g1.Destroy() + g1.Destroy("TestGenerations") expectGet(t, h2, g2, "res") // With all generations destroyed, key should be re-evaluated. - g2.Destroy() + g2.Destroy("TestGenerations") g3 := s.Generation("g3") h3 := g3.Bind("key", func(context.Context, memoize.Arg) interface{} { return "new res" }, nil) expectGet(t, h3, g3, "new res") @@ -89,7 +89,7 @@ func TestCleanup(t *testing.T) { g2 := s.Generation("g2") g2.Inherit(h1, h2) - g1.Destroy() + g1.Destroy("TestCleanup") expectGet(t, h1, g2, &v1) expectGet(t, h2, g2, &v2) for k, v := range map[string]*bool{"key1": &v1, "key2": &v2} { @@ -97,7 +97,7 @@ func TestCleanup(t *testing.T) { t.Errorf("after destroying g1, bound value %q is cleaned up", k) } } - g2.Destroy() + g2.Destroy("TestCleanup") if got, want := v1, false; got != want { t.Error("after destroying g2, v1 is cleaned up") }