From 2d900332d471108407c9c9d12b2437fefdabb6bf Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 10 May 2023 09:51:54 +0200 Subject: [PATCH 1/4] ethdb/pebble: prevent shutdown-panic --- ethdb/pebble/pebble.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/ethdb/pebble/pebble.go b/ethdb/pebble/pebble.go index 5be4c1af731..ff5237a293a 100644 --- a/ethdb/pebble/pebble.go +++ b/ethdb/pebble/pebble.go @@ -70,7 +70,7 @@ type Database struct { seekCompGauge metrics.Gauge // Gauge for tracking the number of table compaction caused by read opt manualMemAllocGauge metrics.Gauge // Gauge for tracking amount of non-managed memory currently allocated - quitLock sync.Mutex // Mutex protecting the quit channel access + quitLock sync.RWMutex // Mutex protecting the quit channel access quitChan chan chan error // Quit channel to stop the metrics collection before closing the database log log.Logger // Contextual logger tracking the database path @@ -238,6 +238,11 @@ func (d *Database) Close() error { // Has retrieves if a key is present in the key-value store. func (d *Database) Has(key []byte) (bool, error) { + d.quitLock.RLock() + defer d.quitLock.RUnlock() + if d.quitChan == nil { + return false, pebble.ErrClosed + } _, closer, err := d.db.Get(key) if err == pebble.ErrNotFound { return false, nil @@ -250,6 +255,11 @@ func (d *Database) Has(key []byte) (bool, error) { // Get retrieves the given key if it's present in the key-value store. func (d *Database) Get(key []byte) ([]byte, error) { + d.quitLock.RLock() + defer d.quitLock.RUnlock() + if d.quitChan == nil { + return nil, pebble.ErrClosed + } dat, closer, err := d.db.Get(key) if err != nil { return nil, err From 680fa2a09ba3efb4b9f197d04b62a613c0f38e93 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 10 May 2023 15:22:29 +0200 Subject: [PATCH 2/4] ethdb/pebble: dedicated closed-flag --- ethdb/pebble/pebble.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/ethdb/pebble/pebble.go b/ethdb/pebble/pebble.go index ff5237a293a..c101f74b0aa 100644 --- a/ethdb/pebble/pebble.go +++ b/ethdb/pebble/pebble.go @@ -72,6 +72,7 @@ type Database struct { quitLock sync.RWMutex // Mutex protecting the quit channel access quitChan chan chan error // Quit channel to stop the metrics collection before closing the database + closed bool // keep track of whether we're Closed log log.Logger // Contextual logger tracking the database path @@ -221,18 +222,19 @@ func New(file string, cache int, handles int, namespace string, readonly bool) ( func (d *Database) Close() error { d.quitLock.Lock() defer d.quitLock.Unlock() - // Allow double closing, simplifies things - if d.quitChan == nil { + if d.closed { return nil } - errc := make(chan error) - d.quitChan <- errc - if err := <-errc; err != nil { - d.log.Error("Metrics collection failed", "err", err) + d.closed = true + if d.quitChan != nil { + errc := make(chan error) + d.quitChan <- errc + if err := <-errc; err != nil { + d.log.Error("Metrics collection failed", "err", err) + } + d.quitChan = nil } - d.quitChan = nil - return d.db.Close() } @@ -240,7 +242,7 @@ func (d *Database) Close() error { func (d *Database) Has(key []byte) (bool, error) { d.quitLock.RLock() defer d.quitLock.RUnlock() - if d.quitChan == nil { + if d.closed { return false, pebble.ErrClosed } _, closer, err := d.db.Get(key) @@ -257,7 +259,7 @@ func (d *Database) Has(key []byte) (bool, error) { func (d *Database) Get(key []byte) ([]byte, error) { d.quitLock.RLock() defer d.quitLock.RUnlock() - if d.quitChan == nil { + if d.closed { return nil, pebble.ErrClosed } dat, closer, err := d.db.Get(key) From 34e67bdf2a92d531ed729f4e1c4a2269a23c63c9 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 10 May 2023 15:42:02 +0200 Subject: [PATCH 3/4] ethdb: better test coverage on post-close behaviour --- ethdb/dbtest/testsuite.go | 26 ++++++++++++++++++++++++++ ethdb/memorydb/memorydb.go | 3 +++ ethdb/pebble/pebble.go | 19 ++++++++++++++++++- 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/ethdb/dbtest/testsuite.go b/ethdb/dbtest/testsuite.go index e455215cb0a..d82dbd69928 100644 --- a/ethdb/dbtest/testsuite.go +++ b/ethdb/dbtest/testsuite.go @@ -376,6 +376,32 @@ func TestDatabaseSuite(t *testing.T, New func() ethdb.KeyValueStore) { } } }) + + t.Run("OperatonsAfterClose", func(t *testing.T) { + db := New() + db.Put([]byte("key"), []byte("value")) + db.Close() + if _, err := db.Get([]byte("key")); err == nil { + t.Fatalf("expected error on Get after Close") + } + if _, err := db.Has([]byte("key")); err == nil { + t.Fatalf("expected error on Get after Close") + } + if err := db.Put([]byte("key2"), []byte("value2")); err == nil { + t.Fatalf("expected error on Put after Close") + } + if err := db.Delete([]byte("key")); err == nil { + t.Fatalf("expected error on Delete after Close") + } + + b := db.NewBatch() + if err := b.Put([]byte("batchkey"), []byte("batchval")); err != nil { + t.Fatalf("expected no error on batch.Put after Close, got %v", err) + } + if err := b.Write(); err == nil { + t.Fatalf("expected error on batch.Write after Close") + } + }) } // BenchDatabaseSuite runs a suite of benchmarks against a KeyValueStore database diff --git a/ethdb/memorydb/memorydb.go b/ethdb/memorydb/memorydb.go index 7e4fd7e5e7f..2b36523b6f2 100644 --- a/ethdb/memorydb/memorydb.go +++ b/ethdb/memorydb/memorydb.go @@ -244,6 +244,9 @@ func (b *batch) Write() error { b.db.lock.Lock() defer b.db.lock.Unlock() + if b.db.db == nil { + return errMemorydbClosed + } for _, keyvalue := range b.writes { if keyvalue.delete { delete(b.db.db, string(keyvalue.key)) diff --git a/ethdb/pebble/pebble.go b/ethdb/pebble/pebble.go index c101f74b0aa..4a4802010fe 100644 --- a/ethdb/pebble/pebble.go +++ b/ethdb/pebble/pebble.go @@ -274,11 +274,21 @@ func (d *Database) Get(key []byte) ([]byte, error) { // Put inserts the given value into the key-value store. func (d *Database) Put(key []byte, value []byte) error { + d.quitLock.RLock() + defer d.quitLock.RUnlock() + if d.closed { + return pebble.ErrClosed + } return d.db.Set(key, value, pebble.NoSync) } // Delete removes the key from the key-value store. func (d *Database) Delete(key []byte) error { + d.quitLock.RLock() + defer d.quitLock.RUnlock() + if d.closed { + return pebble.ErrClosed + } return d.db.Delete(key, nil) } @@ -286,7 +296,8 @@ func (d *Database) Delete(key []byte) error { // database until a final write is called. func (d *Database) NewBatch() ethdb.Batch { return &batch{ - b: d.db.NewBatch(), + b: d.db.NewBatch(), + db: d, } } @@ -493,6 +504,7 @@ func (d *Database) meter(refresh time.Duration) { // when Write is called. A batch cannot be used concurrently. type batch struct { b *pebble.Batch + db *Database size int } @@ -517,6 +529,11 @@ func (b *batch) ValueSize() int { // Write flushes any accumulated data to disk. func (b *batch) Write() error { + b.db.quitLock.RLock() + defer b.db.quitLock.RUnlock() + if b.db.closed { + return pebble.ErrClosed + } return b.b.Commit(pebble.NoSync) } From 3230d1cb9682d3f5d99e5ff2c74fbd8a0eb91be6 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 10 May 2023 09:43:08 -0400 Subject: [PATCH 4/4] Update ethdb/pebble/pebble.go Co-authored-by: Marius van der Wijden --- ethdb/pebble/pebble.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethdb/pebble/pebble.go b/ethdb/pebble/pebble.go index 4a4802010fe..e5380847776 100644 --- a/ethdb/pebble/pebble.go +++ b/ethdb/pebble/pebble.go @@ -70,7 +70,7 @@ type Database struct { seekCompGauge metrics.Gauge // Gauge for tracking the number of table compaction caused by read opt manualMemAllocGauge metrics.Gauge // Gauge for tracking amount of non-managed memory currently allocated - quitLock sync.RWMutex // Mutex protecting the quit channel access + quitLock sync.RWMutex // Mutex protecting the quit channel and the closed flag quitChan chan chan error // Quit channel to stop the metrics collection before closing the database closed bool // keep track of whether we're Closed