Avoid ReadOptions mutated by reference release in iterator #172
Conversation
being accidentally released (mainly due to missing call of Destroy) after creating iterator
db.go
Outdated
| @@ -1146,7 +1146,7 @@ func (db *DB) NewIterator(opts *ReadOptions) *Iterator { | |||
| // that uses the ReadOptions given. | |||
| func (db *DB) NewIteratorCF(opts *ReadOptions, cf *ColumnFamilyHandle) *Iterator { | |||
| cIter := C.rocksdb_create_iterator_cf(db.c, opts.c, cf.c) | |||
| return newNativeIterator(cIter) | |||
| return newNativeIteratorWithTS(cIter, opts.timestamp) | |||
There was a problem hiding this comment.
there are a few other bytes fields in ReadOptions, need to handle in a similar way?
There was a problem hiding this comment.
yes, if it's ok with this direction
mmsqe
changed the title
linxGnu
changed the title
iterator.go
Outdated
| func newNativeIteratorWithReadOptions(c *C.rocksdb_iterator_t, opts *ReadOptions) *Iterator { | ||
| return &Iterator{ | ||
| c: c, | ||
| iterUpperBound: opts.iterUpperBound, |
There was a problem hiding this comment.
Please clone the byte slice as well
There was a problem hiding this comment.
It's by purpose to hold the reference of byte slice in iterator. So to ensure they're not released early when using iterator.
There was a problem hiding this comment.
@mmsqe I mean you must clone the slice, not just keep the reference.
Avoiding accidentally release opts is just one aspect. I would like to avoid data-race as well. In this way, modifying opts.iterUpperBound (outside) will not harm the iterator!
iterUpperBound: clone(opts.iterUpperBound)
func clone(b []byte) []byte {
cloned := make([]byte, len(b))
copy(cloned, b)
return cloned
}There was a problem hiding this comment.
It'll lost the reference, you might try TestIteratorCFWithTSWhenNoDestroy with larger total
There was a problem hiding this comment.
of course, you must use cloned slice as reference. In another word, please clone opts, use cloned one as native iterator's params.
iterator.go
Outdated
| iterUpperBound: opts.iterUpperBound, | ||
| iterLowerBound: opts.iterLowerBound, | ||
| timestamp: opts.timestamp, | ||
| timestampStart: opts.timestampStart, |
It's to avoid golang gc free slices that's referenced in rocksdb. |
|
Yes, and also avoid data race as well if other routines modify opts.slices* while one routine is iterating. In this way, I think we should clone |
@linxGnu Cloning the slice would defeat the reference holding (in iterator) as the original pointer is released right after clone. We do not read nor modify the bytes in iterator at all so it won't cause data race. |
|
Then LGTM. Will find a better way later with cloning whole |
|
@yihuang could you please help reviewing the PR again. Thank you |
iterator.go
Outdated
| @@ -25,14 +25,29 @@ import ( | |||
| // return err | |||
| // } | |||
| type Iterator struct { | |||
| c *C.rocksdb_iterator_t | |||
| c *C.rocksdb_iterator_t | |||
| iterUpperBound []byte | |||
There was a problem hiding this comment.
we can reference the *ReadOptions directly, slightly simpler and future proof if we'll add new slices later.
1. it's not necessary 2. user may want to keep reference it indirectly ref: linxGnu#172
1. it's not necessary 2. user may want to keep reference it indirectly ref: linxGnu#172
1. it's not necessary 2. user may want to keep reference it indirectly ref: linxGnu#172
1. it's not necessary 2. user may want to keep reference it indirectly ref: linxGnu#172
avoid the reference of bytes properties in ReadOptions being accidentally released (mainly due to missing call of Destroy) after creating iterator