-
Notifications
You must be signed in to change notification settings - Fork 2
fix: missing KVS close method (#18) #20
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,28 +2,31 @@ package pot | |
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
|
|
||
| "github.com/ethersphere/proximity-order-trie/pkg/elements" | ||
| ) | ||
|
|
||
| // Index represents a mutable pot | ||
| type Index struct { | ||
| mode elements.Mode // mode | ||
| read chan elements.Node // hands out current root for reads | ||
| write chan elements.Node // hands out current root for writes and locks | ||
| root chan elements.Node // channel for new roots | ||
| quit chan struct{} // closing this channel signals quit | ||
| mode elements.Mode // mode | ||
| read chan elements.Node // hands out current root for reads | ||
| write chan elements.Node // hands out current root for writes and locks | ||
| root chan elements.Node // channel for new roots | ||
| quit chan struct{} // closing this channel signals quit | ||
| closed bool | ||
| } | ||
|
|
||
| // New constructs a new mutable pot | ||
| func New(mode elements.Mode) (*Index, error) { | ||
| idx := &Index{ | ||
| mode: mode, | ||
| read: make(chan elements.Node), | ||
| write: make(chan elements.Node), | ||
| root: make(chan elements.Node), | ||
| quit: make(chan struct{}), | ||
| mode: mode, | ||
| read: make(chan elements.Node), | ||
| write: make(chan elements.Node), | ||
| root: make(chan elements.Node), | ||
| quit: make(chan struct{}), | ||
| closed: false, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this assignment is unnecessary since the zero value of boolean is false |
||
| } | ||
|
|
||
| root := idx.mode.New() | ||
|
|
@@ -34,11 +37,12 @@ func New(mode elements.Mode) (*Index, error) { | |
| // NewReference constructs a new mutable pot from a reference | ||
| func NewReference(ctx context.Context, mode elements.Mode, ref []byte) (*Index, error) { | ||
| idx := &Index{ | ||
| mode: mode, | ||
| read: make(chan elements.Node), | ||
| write: make(chan elements.Node), | ||
| root: make(chan elements.Node), | ||
| quit: make(chan struct{}), | ||
| mode: mode, | ||
| read: make(chan elements.Node), | ||
| write: make(chan elements.Node), | ||
| root: make(chan elements.Node), | ||
| quit: make(chan struct{}), | ||
| closed: false, | ||
| } | ||
|
|
||
| root, loaded, err := idx.mode.Load(ctx, ref) | ||
|
|
@@ -60,6 +64,7 @@ func (idx *Index) muxProcess(root elements.Node) { | |
| for { | ||
| select { | ||
| case <-quit: | ||
| idx.closed = true | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a write write race with close |
||
| return | ||
| case idx.read <- root: // | ||
| case write <- root: // write locks the pot for writes | ||
|
|
@@ -74,7 +79,7 @@ func (idx *Index) muxProcess(root elements.Node) { | |
|
|
||
| // Add inserts an entry to the mutable pot | ||
| func (idx *Index) Add(ctx context.Context, e elements.Entry) error { | ||
| return idx.Update(ctx, e.Key(), &e ) | ||
| return idx.Update(ctx, e.Key(), &e) | ||
| } | ||
|
|
||
| // Delete removes the entry at the given key from the mutable pot | ||
|
|
@@ -86,6 +91,10 @@ func (idx *Index) Delete(ctx context.Context, k []byte) error { | |
| func (idx *Index) Update(ctx context.Context, k []byte, e *elements.Entry) error { | ||
| var root elements.Node | ||
|
|
||
| if idx.closed { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is read/write race on the boolean closed |
||
| return errors.New("trie closed") | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but it is even worse cos if Close is called when we are here concurrently then the following select will hang on line 114 until the context cancels :)) |
||
| // get the pot root and capture the write lock | ||
| select { | ||
| case <-ctx.Done(): | ||
|
|
@@ -112,6 +121,11 @@ func (idx *Index) Update(ctx context.Context, k []byte, e *elements.Entry) error | |
|
|
||
| // Find retrieves the entry at the given key from the mutable pot or gives elements.ErrNotFound | ||
| func (idx *Index) Find(ctx context.Context, k []byte) (elements.Entry, error) { | ||
|
|
||
| if idx.closed { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. exact same as update func |
||
| return nil, errors.New("trie closed") | ||
| } | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
|
|
@@ -122,20 +136,29 @@ func (idx *Index) Find(ctx context.Context, k []byte) (elements.Entry, error) { | |
|
|
||
| // Iterate wraps the underlying pot's iterator | ||
| func (idx *Index) Iterate(ctx context.Context, p, k []byte, f func(elements.Entry) (stop bool, err error)) error { | ||
| if idx.closed { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
| return errors.New("trie closed") | ||
| } | ||
| return elements.Iterate(ctx, elements.NewAt(0, <-idx.read), p, k, idx.mode, f) | ||
| } | ||
|
|
||
| // Size returns the size (number of entries) of the pot | ||
| func (idx *Index) Size() int { | ||
| func (idx *Index) Size() (int, error) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so here you dont need to return an error at all and this is where you should see that the pattern is wrong. So there is no reason one cannot call Size on a closed pot |
||
| if idx.closed { | ||
| return 0, errors.New("trie closed") | ||
| } | ||
| root := <-idx.read | ||
| if root == nil { | ||
| return 0 | ||
| return 0, nil | ||
| } | ||
| return root.Size() | ||
| return root.Size(), nil | ||
| } | ||
|
|
||
| // Save calls the mode specific save method for the root node | ||
| func (idx *Index) Save(ctx context.Context) ([]byte, error) { | ||
| if idx.closed { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. like Size, no reason not to allow Save on a closed |
||
| return nil, errors.New("trie closed") | ||
| } | ||
| root := <-idx.read | ||
| if root.Empty() { | ||
| return nil, fmt.Errorf("root node is nil") | ||
|
|
@@ -145,12 +168,19 @@ func (idx *Index) Save(ctx context.Context) ([]byte, error) { | |
|
|
||
| // Close quits the process loop and closes the mode | ||
| func (idx *Index) Close() error { | ||
| if idx.closed { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you are protecting here from a concurrent call to Close then it is not gonna work and you can still get a close on closed channel error |
||
| return errors.New("trie closed") | ||
| } | ||
| close(idx.quit) | ||
| idx.closed = true | ||
| return nil | ||
| } | ||
|
|
||
| // String pretty prints the current state of the pot | ||
| func (idx *Index) String() string { | ||
| func (idx *Index) String() (string, error) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as Size, Save : no reason why not |
||
| if idx.closed { | ||
| return "", errors.New("trie closed") | ||
| } | ||
| root := <-idx.read | ||
| return elements.NewAt(0, root).String() | ||
| return elements.NewAt(0, root).String(), nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok so this is not needed, in fact checking this field creates a race condition if Close is called concurrently with any other function that checks it.