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.

Sign up for GitHub

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

fix: Prevent newExporter crash if tree.ndb is nil #622

Merged
merged 8 commits into from
Nov 25, 2022
+36 −13
Merged

fix: Prevent newExporter crash if tree.ndb is nil #622

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
055432e
Prevent newExporter crash if tree.ndb is nil
chillyvee Nov 20, 2022
8db8b72
ImmutableTree.Export() returns error if newExporter is given nil argu…
chillyvee Nov 22, 2022
46067ec
Wrap newExporter errors into ErrNotInitalizedTree
chillyvee Nov 22, 2022
e856bd2
change from requested Wrap to fmt.Errorf
chillyvee Nov 24, 2022
ec2b499
Merge branch 'master' into cv_rescue_tree_ndb_nil_master
tac0turtle Nov 24, 2022
f62821d
fix benchmark test
chillyvee Nov 24, 2022
6fb6de0
go fumpting
chillyvee Nov 24, 2022
5b9853c
remove build for previously removed iavlserver
chillyvee Nov 24, 2022
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
10 changes: 10 additions & 0 deletions export.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package iavl
import (
"context"
"errors"
"fmt"
)

// exportBufferSize is the number of nodes to buffer in the exporter. It improves throughput by
Expand Down Expand Up @@ -34,6 +35,15 @@ type Exporter struct {

// NewExporter creates a new Exporter. Callers must call Close() when done.
func newExporter(tree *ImmutableTree) *Exporter {
if tree == nil {
return nil
}
// CV Prevent crash on incrVersionReaders if tree.ndb == nil
if tree.ndb == nil {
fmt.Printf("iavl/export newExporter failed to create because tree.ndb is nil\n")
Copy link
Collaborator

Choose a reason for hiding this comment

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

this function should return error. Doing fmt.Printf doesn't make sense here.

Copy link
Member

Choose a reason for hiding this comment

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

agree, lets break the api.

Copy link
Contributor Author

@chillyvee chillyvee Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Could just return (nil, err) with fmt.Errorf to start with. Then any more complicated error handling can be followed up in other commits/improvment?

Or is there some error standard you're trying to maintain?

Copy link
Member

Choose a reason for hiding this comment

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

yea that makes sense, we would then handle this in the application

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning error now, updated api/test

return nil
}

ctx, cancel := context.WithCancel(context.Background())
exporter := &Exporter{
tree: tree,
Expand Down