From d1d80624c4a2d39615649c60c297f08a923fcb8f Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 23 Mar 2020 13:59:56 -0700 Subject: [PATCH 1/2] fix: make close work again Reference pstoremem components by pointer to: 1. Avoid copying locks around on construction. 2. Avoid copying these around when calling `weakClose`. 3. Ensures that they all implement the `io.Closer` interface (it's implemented on the pointer, not the value). Technically, we could have just taken a reference when calling `weakClose`, but this is cleaner. --- pstoremem/peerstore.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pstoremem/peerstore.go b/pstoremem/peerstore.go index f16887d..113b1d7 100644 --- a/pstoremem/peerstore.go +++ b/pstoremem/peerstore.go @@ -11,20 +11,20 @@ import ( type pstoremem struct { peerstore.Metrics - memoryKeyBook - memoryAddrBook - memoryProtoBook - memoryPeerMetadata + *memoryKeyBook + *memoryAddrBook + *memoryProtoBook + *memoryPeerMetadata } // NewPeerstore creates an in-memory threadsafe collection of peers. func NewPeerstore() *pstoremem { return &pstoremem{ Metrics: pstore.NewMetrics(), - memoryKeyBook: *NewKeyBook(), - memoryAddrBook: *NewAddrBook(), - memoryProtoBook: *NewProtoBook(), - memoryPeerMetadata: *NewPeerMetadata(), + memoryKeyBook: NewKeyBook(), + memoryAddrBook: NewAddrBook(), + memoryProtoBook: NewProtoBook(), + memoryPeerMetadata: NewPeerMetadata(), } } From a77c7a4d616430ef0b9471033d4b129f4e69cf6e Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 23 Mar 2020 14:16:16 -0700 Subject: [PATCH 2/2] test: check for goroutine leaks when closing the peerstore --- go.mod | 1 + go.sum | 7 +++++++ pstoremem/inmem_test.go | 33 ++++++++++++++++++++++++++++----- 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index ce69ef7..4aea9a8 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( github.com/multiformats/go-multihash v0.0.13 github.com/pkg/errors v0.9.1 github.com/whyrusleeping/go-keyspace v0.0.0-20160322163242-5b898ac5add1 + go.uber.org/goleak v1.0.0 golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5 // indirect ) diff --git a/go.sum b/go.sum index cc934e2..116ea44 100644 --- a/go.sum +++ b/go.sum @@ -174,6 +174,8 @@ github.com/xordataexchange/crypt v0.0.3-0.20170626215501-b2862e3d0a77/go.mod h1: go.opencensus.io v0.22.3/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw= go.uber.org/atomic v1.4.0 h1:cxzIVoETapQEqDhQu3QfnvXAV4AlzcvUCxkVUFw3+EU= go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= +go.uber.org/goleak v1.0.0 h1:qsup4IcBdlmsnGfqyLl4Ntn3C2XCCuKAE7DwHpScyUo= +go.uber.org/goleak v1.0.0/go.mod h1:8a7PlsEVH3e/a/GLqe5IIrQx6GzcnRmZEufDUTk4A7A= go.uber.org/multierr v1.1.0 h1:HoEmRHQPVSqub6w2z2d2EOVs2fjyFRGyofhKuyDq0QI= go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0= go.uber.org/zap v1.10.0 h1:ORx85nbTijNz8ljznvCMR1ZBIPKFn3jQrag10X2AsuM= @@ -188,6 +190,8 @@ golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= golang.org/x/lint v0.0.0-20190227174305-5b3e6a55c961/go.mod h1:wehouNa3lNwaWXcvxsM5YxQ5yQlVC4a0KAMCusXpPoU= golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= +golang.org/x/lint v0.0.0-20190930215403-16217165b5de h1:5hukYrvBGR8/eNkX5mdUezrA6JiaEZDtJb9Ei+1LlBs= +golang.org/x/lint v0.0.0-20190930215403-16217165b5de/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -201,6 +205,7 @@ golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAG golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190227155943-e225da77a7e6/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= @@ -222,6 +227,8 @@ golang.org/x/tools v0.0.0-20181130052023-1c3d964395ce/go.mod h1:n7NCudcB/nEzxVGm golang.org/x/tools v0.0.0-20190114222345-bf090417da8b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20190226205152-f727befe758c/go.mod h1:9Yl7xja0Znq3iFh3HoIrodX9oNMXvdceNzlUR8zjMvY= golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs= +golang.org/x/tools v0.0.0-20191108193012-7d206e10da11 h1:Yq9t9jnGoR+dBuitxdo9l6Q7xh/zOyNnYUtDKaQ3x0E= +golang.org/x/tools v0.0.0-20191108193012-7d206e10da11/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/appengine v1.1.0/go.mod h1:EbEs0AVv82hx2wNQdGPgUI5lhzA/G0D9YwlJXL52JkM= google.golang.org/appengine v1.4.0/go.mod h1:xpcJRLb0r/rnEns0DIKYYv+WjYCduHsrkT7/EB5XEv4= diff --git a/pstoremem/inmem_test.go b/pstoremem/inmem_test.go index 4e34756..7491a93 100644 --- a/pstoremem/inmem_test.go +++ b/pstoremem/inmem_test.go @@ -5,34 +5,57 @@ import ( pstore "github.com/libp2p/go-libp2p-core/peerstore" pt "github.com/libp2p/go-libp2p-peerstore/test" + + "go.uber.org/goleak" ) +func TestFuzzInMemoryPeerstore(t *testing.T) { + // Just create and close a bunch of peerstores. If this leaks, we'll + // catch it in the leak check below. + for i := 0; i < 100; i++ { + ps := NewPeerstore() + ps.Close() + } +} + func TestInMemoryPeerstore(t *testing.T) { pt.TestPeerstore(t, func() (pstore.Peerstore, func()) { - return NewPeerstore(), nil + ps := NewPeerstore() + return ps, func() { ps.Close() } }) } func TestInMemoryAddrBook(t *testing.T) { pt.TestAddrBook(t, func() (pstore.AddrBook, func()) { - return NewAddrBook(), nil + ps := NewPeerstore() + return ps, func() { ps.Close() } }) } func TestInMemoryKeyBook(t *testing.T) { pt.TestKeyBook(t, func() (pstore.KeyBook, func()) { - return NewKeyBook(), nil + ps := NewPeerstore() + return ps, func() { ps.Close() } }) } func BenchmarkInMemoryPeerstore(b *testing.B) { pt.BenchmarkPeerstore(b, func() (pstore.Peerstore, func()) { - return NewPeerstore(), nil + ps := NewPeerstore() + return ps, func() { ps.Close() } }, "InMem") } func BenchmarkInMemoryKeyBook(b *testing.B) { pt.BenchmarkKeyBook(b, func() (pstore.KeyBook, func()) { - return NewKeyBook(), nil + ps := NewPeerstore() + return ps, func() { ps.Close() } }) } + +func TestMain(m *testing.M) { + goleak.VerifyTestMain( + m, + goleak.IgnoreTopFunction("github.com/ipfs/go-log/writer.(*MirrorWriter).logRoutine"), + ) +}