Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jevolk opened this issue Nov 8, 2018 · 0 comments
Open

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

jevolk opened this issue Nov 8, 2018 · 0 comments
Labels
up-for-grabs Up for grabs

Comments

@jevolk
Copy link
Contributor

jevolk commented Nov 8, 2018

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<std::mutex> 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<port::Mutex> 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.

@jevolk jevolk changed the title Various use of std thread primitives bypass rocksdb::port and rocksdb::Env Use of std thread primitives bypass rocksdb::port and rocksdb::Env Nov 8, 2018
@ajkr ajkr added the up-for-grabs Up for grabs label Nov 20, 2018
jevolk added a commit to jevolk/charybdis that referenced this issue Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
up-for-grabs Up for grabs
Projects
None yet
Development

No branches or pull requests

2 participants