Use HandleOrRuntime to allow alloydb/ethersdb to hold a custom runtime#1576
Use HandleOrRuntime to allow alloydb/ethersdb to hold a custom runtime#1576rakita merged 6 commits intobluealloy:mainfrom
Conversation
| { | ||
| match self { | ||
| Self::Handle(handle) => tokio::task::block_in_place(move || handle.block_on(f)), | ||
| Self::Runtime(rt) => rt.block_on(f), |
There was a problem hiding this comment.
this will panic if called within async execution context
There was a problem hiding this comment.
Yes, that’s intended. Passing a runtime mostly would be the sync code. If user is within async execution context, they should call new directly instead.
There was a problem hiding this comment.
As stated above, if user is in async context, they should call new as #1557 suggests. If user is in sync context, they should provide a runtime, mostly current_thread runtime.
If user intends to mix different contexts, they should be careful by themselves as it is known to be bad practice and cause problems hard to tackle.
| pub fn with_runtime( | ||
| client: Arc<M>, | ||
| block_number: Option<BlockId>, | ||
| runtime: Runtime, |
There was a problem hiding this comment.
so the caller is responsible for creating a new runtime?
in which case they could also pass in the handle?
There was a problem hiding this comment.
Make sense, we can add a new function and document the behavior.
|
|
||
| use super::utils::HandleOrRuntime; | ||
|
|
||
| #[derive(Debug, Clone)] |
There was a problem hiding this comment.
@wtdcode would you explain why Clone has been removed? I'm slightly new in Rust and want to learn the technical concept behind this decision
There was a problem hiding this comment.
Because runtime doesn’t implement Clone and it doesn’t make too much sense cloning the db. Or, you could wrap it with an Rc or Arc.
What’s your use case?
There was a problem hiding this comment.
thanks
In my case, I define an ethersDB instance and by using it, I define a cacheDB. I add some base data (deploy contract, change some slots, etc) to cacheDB and after that, by cloning it, I send the same copies to multiple threads.
For now, I get an error (when cloning cacheDB) complains about ethersDB (as internal object of cacheDB) cannot be cloned.
because inside separate threads, I need to write to db, I tested not cloning and use Arc/Mutex. but the performance was worse than cloning.
There was a problem hiding this comment.
thanks In my case, I define an ethersDB instance and by using it, I define a cacheDB. I add some base data (deploy contract, change some slots, etc) to cacheDB and after that, by cloning it, I send the same copies to multiple threads.
For now, I get an error (when cloning cacheDB) complains about ethersDB (as internal object of cacheDB) cannot be cloned.
because inside separate threads, I need to write to db, I tested not cloning and use Arc/Mutex. but the performance was worse than cloning.
It's not really make sense to clone AlloyDB or EthersDB. Clone was also added by me in a previous PR, but I recently found it was a mistake. Generally, it is due to:
- Both
ProviderandMiddlewareare only safe to clone within the same runtime. If you send it across runtimes (or threads), many internal things and assumptions could break. - Even if ensure you are in the same async context all the time or just sync code, it just doesn't make sense to clone the EthersDB because it saves nothing.
To your use case which is similar to mine, my suggestion is:
- Share a single
Arc<Mutex<CacheDB<EthersDB>>>across all threads. This avoids duplicate caching for each thread and can speed up your application overall. - Clone
CacheDBmembers (note they are public) except theEthersDBand create a newEthersDBinstead. This generously is a manual "Clone" implementation.
There was a problem hiding this comment.
There was a problem hiding this comment.
Note with_db accepts borrows.
There was a problem hiding this comment.
you mean with_ref_db ? I want to update the cache in each transact from multiple threads

Following advice from @DaniPopes, this PR introduce
HandleOrRuntimefor bothEthersDBandAlloyDBto avoid creating runtime for every call. Compared to #1557, this implementation also allows synchronous code to continue using both implementation by callingAlloyDB::with_runtime(..., ..., new_current_thread()), which reduces break changes.This PR shall address the concerns of @DaniPopes while keeping the compatibility.
with_runtime.Note with this PR, we could also support current thread runtime now.