Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core/rawdb: fix cornercase shutdown behaviour in freezer #26485

Merged
merged 8 commits into from
Jan 16, 2023
Prev Previous commit
Next Next commit
core/rawdb: handle sync after close
  • Loading branch information
holiman committed Jan 12, 2023
commit ebe49f4252fec4a119e029efd6c2ea6c4e741afc
3 changes: 1 addition & 2 deletions core/rawdb/chain_freezer.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,13 @@ func newChainFreezer(datadir string, namespace string, readonly bool, maxTableSi

// Close closes the chain freezer instance and terminates the background thread.
func (f *chainFreezer) Close() error {
err := f.Freezer.Close()
select {
case <-f.quit:
default:
close(f.quit)
}
f.wg.Wait()
return err
return f.Freezer.Close()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

}

// freeze is a background thread that periodically checks the blockchain for any
Expand Down
4 changes: 3 additions & 1 deletion core/rawdb/freezer_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,9 @@ func (t *freezerTable) advanceHead() error {
func (t *freezerTable) Sync() error {
t.lock.Lock()
defer t.lock.Unlock()

if t.index == nil || t.head == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also check if t.meta is nil.

return errClosed
}
var err error
trackError := func(e error) {
if e != nil && err == nil {
Expand Down
48 changes: 48 additions & 0 deletions core/rawdb/freezer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -407,3 +407,51 @@ func TestRenameWindows(t *testing.T) {
t.Errorf("unexpected file contents. Got %v\n", buf)
}
}

func TestFreezerCloseSync(t *testing.T) {
t.Parallel()

// Create test data.
var valuesRaw [][]byte
var valuesRLP []*big.Int
for x := 0; x < 100; x++ {
v := getChunk(256, x)
valuesRaw = append(valuesRaw, v)
iv := big.NewInt(int64(x))
iv = iv.Exp(iv, iv, nil)
valuesRLP = append(valuesRLP, iv)
}

tables := map[string]bool{"raw": true, "rlp": false}
f, _ := newFreezerForTesting(t, tables)
defer f.Close()

// Commit test data.
_, err := f.ModifyAncients(func(op ethdb.AncientWriteOp) error {
for i := range valuesRaw {
if err := op.AppendRaw("raw", uint64(i), valuesRaw[i]); err != nil {
return err
}
if err := op.Append("rlp", uint64(i), valuesRLP[i]); err != nil {
return err
}
}
return nil
})
if err != nil {
t.Fatal("ModifyAncients failed:", err)
}
// Now, close and sync. This mimics the behaviour if the node is shut down,
// just as the chain freezer is writing.
// 1: thread-1: chain treezer writes, via freezeRange (holds lock)
// 2: thread-2: Close called, waits for write to finish
// 3: thread-1: finishes writing, releases lock
// 4: thread-2: obtains lock, completes Close()
// 5: thread-1: calls f.Sync()
if err := f.Close(); err != nil {
t.Fatal(err)
}
if err := f.Sync(); err != nil {
t.Fatal(err)
}
Copy link
Contributor Author

@holiman holiman Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I'll just change this, and expect an error, and that the error (or error-string) is "closed", then?

}