facebook / facebook/rocksdb

Use of std thread primitives bypass rocksdb::port and rocksdb::Env

Open
#4,654 0 comments 4 reactions 0 assignees View on GitHub
up-for-grabs
Dominant language
C++
Stars
32.1k
Forks
6.9k
Avg merge
32m
Merged PRs (30d)
1

Description

There are several instances now where stdlib thread interfaces are used directly without being wrapped by `rocksdb::port` or `rocksdb::Env` as they ought to be.

Notable examples:
- https://github.com/facebook/rocksdb/blob/5.16.fb/memtable/write_buffer_manager.cc#L25
`std::mutex` is used here rather than `rocksdb::port::Mutex`. When the embedding adapts the `rocksdb::port` for alternative threading, contending for this `std::mutex` will deadlock the program.

- https://github.com/facebook/rocksdb/blob/5.16.fb/port/port_posix.h#L167
`rocsdb::port` itself should not typedef `Thread` to `std::thread` because that bypasses the `rocksdb::Env` interface for thread-related callbacks. In this case I would advise RocksDB to create their own internal `Thread` object which leads to `rocksdb::Env::StartThread()` et al. An example of where this is a problem is the `rocksdb::util::SstFileManager` where each instance spawns a `port::Thread` thus unconditionally spawning an `std::thread` for an embedding implementing the env callback instead (which is not called).

- Various instances of `std::lock_guard` can be found with a `grep`. The `std::lock_guard` template can be used with any lockable-concept class; in other words it should be `std::lock_guard` throughout RocksDB's code.

Adapting to and optimizing for different environments and platforms has always been a major asset of LevelDB/RocksDB: that is why the `rocksdb::Env` and `rocksdb::port` interfaces are so comprehensive. I hope use of stdlib thread interfaces will remain fully wrapped going forward.

Contributor guide

Open the contributing guide

Research direction

Start by reviewing memtable/write_buffer_manager.cc and port/port_posix.h, then grep for std::lock_guard and other direct standard-thread usage. Check rocksdb::Env::StartThread and the util::SstFileManager entry point to understand the affected interfaces. Done means thread primitives consistently use rocksdb::port or rocksdb::Env without bypassing embedding callbacks.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
databases
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.