From 1a237ccce04716bca67041393293d82ed171937d Mon Sep 17 00:00:00 2001 From: Ricardo Silva Date: Tue, 16 Feb 2021 13:53:07 +0000 Subject: [PATCH 1/7] fix: check if freezer db process already stopped before doing it again --- core/rawdb/freezer.go | 12 +++++++++++- eth/backend.go | 1 + raft/backend.go | 4 ++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/core/rawdb/freezer.go b/core/rawdb/freezer.go index cfb681d3c4..a41ea1f22e 100644 --- a/core/rawdb/freezer.go +++ b/core/rawdb/freezer.go @@ -128,7 +128,17 @@ func newFreezer(datadir string, namespace string) (*freezer, error) { // Close terminates the chain freezer, unmapping all the data files. func (f *freezer) Close() error { - f.quit <- struct{}{} + // TODO ricardolyn: add unit test + // Quorum + // Check if 'f.quit' channel already closed, as freezer.Close() might be called again by Raft when stopping raft service + select { + case f.quit <- struct{}{}: + log.Info("Freezer DB process stopped") + default: + log.Info("Freezer DB process already stopped") + } + // End Quorum + var errs []error for _, table := range f.tables { if err := table.Close(); err != nil { diff --git a/eth/backend.go b/eth/backend.go index aefe3ccf1e..d16eac09af 100644 --- a/eth/backend.go +++ b/eth/backend.go @@ -661,6 +661,7 @@ func (s *Ethereum) Stop() error { s.miner.Stop() s.blockchain.Stop() s.engine.Close() + //TODO ricardolyn: when running on RAFT mode, eth service is also running and chaindb.Close is called here first s.chainDb.Close() s.eventMux.Stop() return nil diff --git a/raft/backend.go b/raft/backend.go index ca3d6644b8..3ee169b5e2 100644 --- a/raft/backend.go +++ b/raft/backend.go @@ -99,6 +99,10 @@ func (service *RaftService) Stop() error { service.minter.stop() service.eventMux.Stop() + // TODO ricardolyn: + // when running on RAFT mode, raft service calls this after eth service. + // after the change of https://github.com/ethereum/go-ethereum/pull/21010/files, means that the freezer process is already stopped by eth service when doing close. + // so when raft service tries to close, it hangs as the channel is alreacy closed. service.chainDb.Close() log.Info("Raft stopped") From d1e8ba6a94a041a81b96f140dcc6799a662c8c14 Mon Sep 17 00:00:00 2001 From: Ricardo Silva Date: Tue, 16 Feb 2021 16:52:44 +0000 Subject: [PATCH 2/7] test: add unit test for the close freezer db todos: removed todos from ricardolyn --- core/rawdb/freezer.go | 10 ++--- core/rawdb/freezer_test.go | 83 ++++++++++++++++++++++++++++++++++++++ eth/backend.go | 1 - go.sum | 1 + raft/backend.go | 5 +-- 5 files changed, 90 insertions(+), 10 deletions(-) create mode 100644 core/rawdb/freezer_test.go diff --git a/core/rawdb/freezer.go b/core/rawdb/freezer.go index a41ea1f22e..41cfd99d3f 100644 --- a/core/rawdb/freezer.go +++ b/core/rawdb/freezer.go @@ -128,18 +128,18 @@ func newFreezer(datadir string, namespace string) (*freezer, error) { // Close terminates the chain freezer, unmapping all the data files. func (f *freezer) Close() error { - // TODO ricardolyn: add unit test + var errs []error + // Quorum - // Check if 'f.quit' channel already closed, as freezer.Close() might be called again by Raft when stopping raft service + // Check if 'f.quit' has subscribers, as freezer.Close() might be called again by Raft when stopping raft service select { case f.quit <- struct{}{}: - log.Info("Freezer DB process stopped") + log.Info("stopping Freezer DB process") default: - log.Info("Freezer DB process already stopped") + errs = append(errs, errors.New("freezer DB process already stopped")) } // End Quorum - var errs []error for _, table := range f.tables { if err := table.Close(); err != nil { errs = append(errs, err) diff --git a/core/rawdb/freezer_test.go b/core/rawdb/freezer_test.go new file mode 100644 index 0000000000..be977f0c4d --- /dev/null +++ b/core/rawdb/freezer_test.go @@ -0,0 +1,83 @@ +package rawdb + +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "testing" + "time" +) + +// Releaser is an autogenerated mock type for the Releaser type +type releaserMock struct { + mock.Mock +} + +// Release provides a mock function with given fields: +func (_m *releaserMock) Release() error { + ret := _m.Called() + + var r0 error + if rf, ok := ret.Get(0).(func() error); ok { + r0 = rf() + } else { + r0 = ret.Error(0) + } + + return r0 +} + +func newFreezerMock(t *testing.T) *freezer { + mockLock := new(releaserMock) + mockFreezer := &freezer{ + tables: make(map[string]*freezerTable), + instanceLock: mockLock, + quit: make(chan struct{}), + } + mockLock.On("Release").Return(nil) + + started := make(chan bool) + go func() { + started <- true + select { + case <-mockFreezer.quit: + // message handled + } + }() + <-started + + return mockFreezer +} + +func Test_freezer_shouldClose_whenHaveSubscribers(t *testing.T) { + // given + mockFreezer := newFreezerMock(t) + + // when + err := mockFreezer.Close() + + // then + assert.Nil(t, err) +} + +func Test_shouldCloseGracefully_whenNoSubscribers(t *testing.T) { + // given + mockFreezer := newFreezerMock(t) + mockFreezer.Close() // should trigger channel close + timeout := time.After(1 * time.Second) + errCh := make(chan error) + + // when + go func() { + errCh <- mockFreezer.Close() + }() + + // then + select { + case <-timeout: + t.Fatal("freezer.Close() timed out") + case err := <-errCh: + assert.NotNil(t, err) + assert.Equal(t, "[freezer DB process already stopped]", err.Error()) + } + +} diff --git a/eth/backend.go b/eth/backend.go index d16eac09af..aefe3ccf1e 100644 --- a/eth/backend.go +++ b/eth/backend.go @@ -661,7 +661,6 @@ func (s *Ethereum) Stop() error { s.miner.Stop() s.blockchain.Stop() s.engine.Close() - //TODO ricardolyn: when running on RAFT mode, eth service is also running and chaindb.Close is called here first s.chainDb.Close() s.eventMux.Stop() return nil diff --git a/go.sum b/go.sum index 2da5e299a6..48625f1978 100644 --- a/go.sum +++ b/go.sum @@ -238,6 +238,7 @@ github.com/steakknife/bloomfilter v0.0.0-20180922174646-6819c0d2a570 h1:gIlAHnH1 github.com/steakknife/bloomfilter v0.0.0-20180922174646-6819c0d2a570/go.mod h1:8OR4w3TdeIHIh1g6EMY5p0gVNOovcWC+1vpc7naMuAw= github.com/steakknife/hamming v0.0.0-20180906055917-c99c65617cd3 h1:njlZPzLwU639dk2kqnCPPv+wNjq7Xb6EfUxe/oX0/NM= github.com/steakknife/hamming v0.0.0-20180906055917-c99c65617cd3/go.mod h1:hpGUWaI9xL8pRQCTXQgocU38Qw1g0Us7n5PxxTwTCYU= +github.com/stretchr/objx v0.1.0 h1:4G4v2dO3VZwixGIRoQ5Lfboy6nUhCyYzaqnIAPPhYs4= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= diff --git a/raft/backend.go b/raft/backend.go index 3ee169b5e2..1731b42981 100644 --- a/raft/backend.go +++ b/raft/backend.go @@ -99,10 +99,7 @@ func (service *RaftService) Stop() error { service.minter.stop() service.eventMux.Stop() - // TODO ricardolyn: - // when running on RAFT mode, raft service calls this after eth service. - // after the change of https://github.com/ethereum/go-ethereum/pull/21010/files, means that the freezer process is already stopped by eth service when doing close. - // so when raft service tries to close, it hangs as the channel is alreacy closed. + // chainDb.Close() handles gracefully if freezedb process is already stopped service.chainDb.Close() log.Info("Raft stopped") From 1fba153b0683b4c4e54dc826e3c074fe88cd779a Mon Sep 17 00:00:00 2001 From: Ricardo Silva Date: Tue, 16 Feb 2021 16:58:51 +0000 Subject: [PATCH 3/7] lint: fix lint issues --- core/rawdb/freezer_test.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/core/rawdb/freezer_test.go b/core/rawdb/freezer_test.go index be977f0c4d..acdb0a6751 100644 --- a/core/rawdb/freezer_test.go +++ b/core/rawdb/freezer_test.go @@ -1,12 +1,15 @@ package rawdb import ( - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "testing" "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) +// Quorum + // Releaser is an autogenerated mock type for the Releaser type type releaserMock struct { mock.Mock @@ -38,10 +41,7 @@ func newFreezerMock(t *testing.T) *freezer { started := make(chan bool) go func() { started <- true - select { - case <-mockFreezer.quit: - // message handled - } + <-mockFreezer.quit }() <-started @@ -81,3 +81,5 @@ func Test_shouldCloseGracefully_whenNoSubscribers(t *testing.T) { } } + +// End Quorum From 75d307238cd534f79b6488e895758972878c959c Mon Sep 17 00:00:00 2001 From: Ricardo Silva Date: Tue, 16 Feb 2021 17:03:26 +0000 Subject: [PATCH 4/7] tidy: remove unnecessary log --- core/rawdb/freezer.go | 1 - 1 file changed, 1 deletion(-) diff --git a/core/rawdb/freezer.go b/core/rawdb/freezer.go index 41cfd99d3f..73699ea3b9 100644 --- a/core/rawdb/freezer.go +++ b/core/rawdb/freezer.go @@ -134,7 +134,6 @@ func (f *freezer) Close() error { // Check if 'f.quit' has subscribers, as freezer.Close() might be called again by Raft when stopping raft service select { case f.quit <- struct{}{}: - log.Info("stopping Freezer DB process") default: errs = append(errs, errors.New("freezer DB process already stopped")) } From d889e5cf94d4e0476331742731e146190ebc3381 Mon Sep 17 00:00:00 2001 From: Ricardo Silva Date: Tue, 16 Feb 2021 17:05:52 +0000 Subject: [PATCH 5/7] tidy: test file --- core/rawdb/freezer_test.go | 68 +++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/core/rawdb/freezer_test.go b/core/rawdb/freezer_test.go index acdb0a6751..685d0a50ee 100644 --- a/core/rawdb/freezer_test.go +++ b/core/rawdb/freezer_test.go @@ -10,6 +10,40 @@ import ( // Quorum +func Test_freezer_shouldClose_whenHaveSubscribers(t *testing.T) { + // given + mockFreezer := newFreezerMock(t) + + // when + err := mockFreezer.Close() + + // then + assert.Nil(t, err) +} + +func Test_shouldCloseGracefully_whenNoSubscribers(t *testing.T) { + // given + mockFreezer := newFreezerMock(t) + mockFreezer.Close() // should trigger channel close + timeout := time.After(1 * time.Second) + errCh := make(chan error) + + // when + go func() { + errCh <- mockFreezer.Close() + }() + + // then + select { + case <-timeout: + t.Fatal("freezer.Close() timed out") + case err := <-errCh: + assert.NotNil(t, err) + assert.Equal(t, "[freezer DB process already stopped]", err.Error()) + } + +} + // Releaser is an autogenerated mock type for the Releaser type type releaserMock struct { mock.Mock @@ -48,38 +82,4 @@ func newFreezerMock(t *testing.T) *freezer { return mockFreezer } -func Test_freezer_shouldClose_whenHaveSubscribers(t *testing.T) { - // given - mockFreezer := newFreezerMock(t) - - // when - err := mockFreezer.Close() - - // then - assert.Nil(t, err) -} - -func Test_shouldCloseGracefully_whenNoSubscribers(t *testing.T) { - // given - mockFreezer := newFreezerMock(t) - mockFreezer.Close() // should trigger channel close - timeout := time.After(1 * time.Second) - errCh := make(chan error) - - // when - go func() { - errCh <- mockFreezer.Close() - }() - - // then - select { - case <-timeout: - t.Fatal("freezer.Close() timed out") - case err := <-errCh: - assert.NotNil(t, err) - assert.Equal(t, "[freezer DB process already stopped]", err.Error()) - } - -} - // End Quorum From 9d2f05e8ab0c71269097740d20f1e2a2266ff512 Mon Sep 17 00:00:00 2001 From: Ricardo Silva Date: Tue, 16 Feb 2021 17:10:26 +0000 Subject: [PATCH 6/7] tidy: remove missleading comment --- core/rawdb/freezer_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/rawdb/freezer_test.go b/core/rawdb/freezer_test.go index 685d0a50ee..e18a60881e 100644 --- a/core/rawdb/freezer_test.go +++ b/core/rawdb/freezer_test.go @@ -24,7 +24,7 @@ func Test_freezer_shouldClose_whenHaveSubscribers(t *testing.T) { func Test_shouldCloseGracefully_whenNoSubscribers(t *testing.T) { // given mockFreezer := newFreezerMock(t) - mockFreezer.Close() // should trigger channel close + mockFreezer.Close() timeout := time.After(1 * time.Second) errCh := make(chan error) From b59725966917f7f5bd8c3c6df9e1c1a7b95a43d6 Mon Sep 17 00:00:00 2001 From: Ricardo Silva Date: Tue, 16 Feb 2021 18:55:05 +0000 Subject: [PATCH 7/7] tidy: refactored unit test into 1 --- core/rawdb/freezer_test.go | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/core/rawdb/freezer_test.go b/core/rawdb/freezer_test.go index e18a60881e..00e9a97866 100644 --- a/core/rawdb/freezer_test.go +++ b/core/rawdb/freezer_test.go @@ -10,30 +10,20 @@ import ( // Quorum -func Test_freezer_shouldClose_whenHaveSubscribers(t *testing.T) { - // given +func Test_freezer_Close(t *testing.T) { + // Close first time mockFreezer := newFreezerMock(t) - - // when err := mockFreezer.Close() - - // then assert.Nil(t, err) -} -func Test_shouldCloseGracefully_whenNoSubscribers(t *testing.T) { - // given - mockFreezer := newFreezerMock(t) - mockFreezer.Close() + // Close second time should return error but do not hang sending data to freeze.quit channel timeout := time.After(1 * time.Second) errCh := make(chan error) - // when go func() { errCh <- mockFreezer.Close() }() - // then select { case <-timeout: t.Fatal("freezer.Close() timed out")