Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

checkpoint: fix empty map become nil after unmarshall #237

Merged
merged 3 commits into from
Sep 6, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion lightning/checkpoints/checkpoints.go
Original file line number Diff line number Diff line change
Expand Up @@ -741,7 +741,25 @@ func NewFileCheckpointsDB(path string) *FileCheckpointsDB {
// ignore all errors -- file maybe not created yet (and it is fine).
content, err := ioutil.ReadFile(path)
if err == nil {
cpdb.checkpoints.Unmarshal(content)
err2 := cpdb.checkpoints.Unmarshal(content)
if err2 != nil {
log.L().Error("checkpoint file is broken", zap.String("path", path), zap.Error(err2))
}
// FIXME: patch for empty map may need initialize manually, because currently
// FIXME: a map of zero size -> marshall -> unmarshall -> become nil, see checkpoint_test.go
Copy link
Collaborator

Choose a reason for hiding this comment

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

why using FIXME for these comments?

Copy link
Contributor Author

@lance6716 lance6716 Sep 6, 2019

Choose a reason for hiding this comment

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

I think it's an unexpected behaviour of gogo/protobuf's marshall and unmarshall. There's logic to recover a empty map(but didn't execute) in

if m.Engines == nil {
m.Engines = make(map[int32]*EngineCheckpointModel)

If gogo/protobuf fixed this bug, we won't bother to initiate. So I added a FIXME to remind to report bug and check if fixed from gogo/protobuf.

if cpdb.checkpoints.Checkpoints == nil {
cpdb.checkpoints.Checkpoints = map[string]*TableCheckpointModel{}
}
for _, table := range cpdb.checkpoints.Checkpoints {
if table.Engines == nil {
table.Engines = map[int32]*EngineCheckpointModel{}
}
for _, engine := range table.Engines {
if engine.Chunks == nil {
engine.Chunks = map[string]*ChunkCheckpointModel{}
}
}
}
} else {
log.L().Info("open checkpoint file failed, going to create a new one",
zap.String("path", path),
Expand Down
14 changes: 14 additions & 0 deletions lightning/checkpoints/checkpoints_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -288,3 +288,17 @@ func (s *checkpointSuite) TestApplyDiff(c *C) {
},
})
}

func (s *checkpointSuite) TestCheckpointMarshallUnmarshall(c *C) {
path := "/tmp/test-chkp"
lance6716 marked this conversation as resolved.
Show resolved Hide resolved
fileChkp := NewFileCheckpointsDB(path)
fileChkp.checkpoints.Checkpoints["a"] = &TableCheckpointModel{
Status: uint32(CheckpointStatusLoaded),
Engines: map[int32]*EngineCheckpointModel{},
}
fileChkp.Close()

fileChkp2 := NewFileCheckpointsDB(path)
// if not recover empty map explicitly, it will become nil
c.Assert(fileChkp2.checkpoints.Checkpoints["a"].Engines, NotNil)
}