rocksdb Slice()'s might not be Free()'ed

[steveyen]

Just moving a bug from couchbase JIRA to over here...

The github.com/tecbot/gorocksdb API's have a Slice() abstraction that has a Free() method, but that doesn't seem to be invoked by the blevex/rocksdb current adapter. So, this issue is to followup.

This is from code reading/review, not from seeing actual memory leak in the wild. This might be because I've focused on data-loading (bleve-blast) rather than doc update cases.

See also: https://issues.couchbase.com/browse/MB-17401

[mschoch]

Having reviewed this more carefully while rewriting this code for the cockroachdb version I can comment.

There are 2 places where this comes up.

1. Call to Get() - Here we called Data() on the Slice() and return those bytes. This is incorrect and leaks memory since there is no way to know when the caller is done with them. In total, it probably isn't a huge problem because we infrequently call Get(), but we still need to fix it. Because the contract of Get() is such that the caller should own the bytes, the better solution is to copy them here. Instead we should call GetBytes() which makes a copy for Go, and lets Rocksdb free its version.
2. In Iterators, Key() and Value() we also call Data() but in this case it is not a bug. It has to do with the fact that because of RocksDB iterator contract we don't really own these bytes anyway. Further, you can see that the RocksDB wrapper sets "freed" = true when creating the slice:

https://github.com/tecbot/gorocksdb/blob/master/iterator.go#L57

That prevents us from accidentally freeing memory we didn't own. So, even we added a call to Free() it would be a no-op.

I provide all this for informational purposes though, as I'd be more inclined to merge the other PR as I had the fix for 1 in mind at the time.