Patient0 on RocksDB – A persistent key-value store for fast storage environments

Comment on RocksDB – A persistent key-value store for fast storage environments

I'm surprised that the C++ code is not using the RAII idiom in some obvious places.

For example: https://github.com/facebook/rocksdb/blob/master/db/db_impl.c

There are many places with bracketed calls to mutex_.Lock and mutex_.Unlock().

An example:

      mutex_.Unlock();
      LogFlush(options_.info_log);
      env_->SleepForMicroseconds(1000000);
      mutex_.Lock()

Why didn't the authors use the RAII idiom here? Even if there are no exceptions expected, the code would still be simpler and less error prone by using a guard object.

tsewlliw 12y

fixed your link: https://github.com/facebook/rocksdb/blob/master/db/db_impl.c...

Take another look! There's a guard object used at the function scope to ensure the lock is released, and this block is bracketed to release and reacquire the lock, not acquire and release. There may be a case for a guard object that does the release/reacquire, but its definitely not a slam dunk like acquire/release

phunge 12y

Still, that's not exception-safe, correct? If LogFlush or SleepForMicroseconds throws an exception the mutex will be unlocked twice, which pthreads disallows for normal mutexes...

cbsmith 12y

You know, for a second I thought you were wrong, but I changed my mind. This does look like a bug, and a simple on to avoid at that.

It's tough, because Rocks is still highly based on LevelDB, which conforms to Google's coding style guideline, which makes RAII more than a bit tricky to do right.

e12e 12y

That link should be:

https://github.com/facebook/rocksdb/blob/master/db/db_impl.c...

(not db_impl.c but .cc) I was wondering for a while if it was possible to do RAII in idiomatic c99 -- and it appears it isn't (or doesn't make as much sense, anyway).

ExpiredLink 12y

A lot of C++ code essentially is 'C with classes': no RAII, no exception handling.

Comments