fix(store/state): perform potentially blocking RocksDB access in a blocking context#2076
Conversation
…king task The account and nullifier trees may be backed by `RocksDB`, so tree access must not run on an async worker thread directly.
The account state forest may be backed by `RocksDB`, so access must not run on an async worker thread directly.
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Looks fine ito code, I'm just a bit concerned that we're using block_in_place? Should these be spawn_blocking?
| tokio::runtime::Handle::current() | ||
| .block_on(db_update_task)? |
There was a problem hiding this comment.
Should we be using block_on? I guess we're sort of saved because we know this will only happen once, and always sequentially?
There was a problem hiding this comment.
An alternative would be using a oneshot channel for explicit synchronization on the database commit -- that would avoid this ugly block_on() call here.
There was a problem hiding this comment.
I think we can live with these as is until we get proper write isolation.
I've considered that -- but it gets ugly because that way we need an additional |
…rocksdb-access-to-blocking-tasks
|
I'm wondering if we should consider implementing a solution closer to the actual RocksDB code. Something like this (perhaps our own impl) https://crates.io/crates/async-rocksdb? |
The problem is not only the RocksDB layer though... Both |
|
The other thing to consider is that we're actually not doing any async at all in any part of the store. We're using async wrappers for everything but in reality all the actual work is sync. We've sort of done the wrong thing by adding async wrappers and I'd like to go the opposite direction. As in, |
This PR moves runtime access to RocksDB-backed store state structures onto Tokio’s blocking path via
block_in_place().State::innerandState::forestlock access.storage_map_key_cacheupdates on the normal async path since they do not touch the RocksDB-backed forest. Likewise,blockchainoperations are entirely in-memory, so those too are left in async context.Using
block_in_place()is required because usingspawn_blocking()would require the closure to be'static, which in turn would require having an additionalArcwrapper aroundinnerandforest.Closes #1969