Are `&mut self` methods necessary? about rust-rocksdb HOT 6 CLOSED

@kaedroho absolutely! The release I cut today will be a patch for 0.4, and it will also be in master for our upcoming 0.5 release.

from rust-rocksdb.

Indeed, only the underlying native type for DB is thread-safe – so all its methods can be &self, but for the others like WriteBatch, etc., they’ll need &mut self methods where appropriate.

Thanks for sorting this out.

On 13 Nov 2016, at 12:01, Tyler Neely [email protected] wrote:

I'm in favor of making any of the &mut self methods &self if their underlying implementations are thread-safe. From the rocksdb docs:

A database may only be opened by one process at a time. The rocksdb implementation acquires a lock from the operating system to prevent misuse. Within a single process, the same rocksdb::DB object may be safely shared by multiple concurrent threads. I.e., different threads may write into or fetch iterators or call Get on the same database without any external synchronization (the leveldb implementation will automatically do the required synchronization). However other objects (like Iterator and WriteBatch) may require external synchronization. If two threads share such an object, they must protect access to it using their own locking protocol. More details are available in the public header files.
Scanning the source code, I actually need to make a few things that are &self &mut self! (WriteBatch, DBIterator::valid, maybe some others) This may already be causing bugs for users of this library! I'll get on this right now and cut a new release, as safety issues like this deserve rapid attention. I really appreciate you bringing up this issue, even though this outcome may not be the one you were hoping for :)

We can talk about whether we want to provide optional thread-safe access patterns that use RWLock or something under the hood. We need to think about a clean way to support both, but in the mean time I will perform an audit and make the methods we provide safe.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #91 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAEF3GWqtb0emV0n9Da2qtzf0SmEH3N1ks5q9vwDgaJpZM4KvNYg.

from rust-rocksdb.

I'm in favor of making any of the &mut self methods &self if their underlying implementations are thread-safe. From the rocksdb docs:

A database may only be opened by one process at a time. The rocksdb implementation acquires 
a lock from the operating system to prevent misuse. Within a single process, the same 
rocksdb::DB object may be safely shared by multiple concurrent threads. I.e., different threads 
may write into or fetch iterators or call Get on the same database without any external 
synchronization (the leveldb implementation will automatically do the required synchronization). 
However other objects (like Iterator and WriteBatch) may require external synchronization. If two 
threads share such an object, they must protect access to it using their own locking protocol. 
More details are available in the public header files.

Scanning the source code, I actually need to make a few things that are &self &mut self! (WriteBatch, DBIterator::valid, maybe some others) This may already be causing bugs for users of this library! I'll get on this right now and cut a new release, as safety issues like this deserve rapid attention. I really appreciate you bringing up this issue, even though this outcome may not be the one you were hoping for :)

We can talk about whether we want to provide optional thread-safe access patterns that use RWLock or something under the hood. We need to think about a clean way to support both, but in the mean time I will perform an audit and make the methods we provide safe.

from rust-rocksdb.

@ngaut heads up, you may have addressed this already in your code but this is a safety issue you should be aware of if not.

from rust-rocksdb.

This may already be causing bugs for users of this library! I'll get on this right now and cut a new release, as safety issues like this deserve rapid attention.

Would you consider making a stable/0.4.x branch, based on 0.4.2, which we can cherry-pick safety fixes like this to and release as 0.4.3 (and so on).

0.5 currently has some API breakages so I think it's worth including this fix in both a patch for 0.4.x as well as the forthcoming 0.5 release.

from rust-rocksdb.

cut v0.4.2 that uses a RwLock to preserve the API, and in the 0.5.0 branch remove Writeable and have operations on a WriteBatch be &mut self.

https://github.com/spacejam/rust-rocksdb/releases/tag/v0.4.2

#93

from rust-rocksdb.