-
Notifications
You must be signed in to change notification settings - Fork 616
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 HandleOrRuntime to allow alloydb/ethersdb to hold a custom runtime #1576
Changes from 2 commits
00adb4b
fda89ed
f94b1b7
ba20812
e6c8a57
01899bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,27 +2,29 @@ use std::sync::Arc; | |
|
||
use ethers_core::types::{Block, BlockId, TxHash, H160 as eH160, H256, U64 as eU64}; | ||
use ethers_providers::Middleware; | ||
use tokio::runtime::Handle; | ||
use tokio::runtime::{Handle, Runtime}; | ||
|
||
use crate::primitives::{AccountInfo, Address, Bytecode, B256, U256}; | ||
use crate::{Database, DatabaseRef}; | ||
|
||
#[derive(Debug, Clone)] | ||
use super::utils::HandleOrRuntime; | ||
|
||
#[derive(Debug)] | ||
pub struct EthersDB<M: Middleware> { | ||
client: Arc<M>, | ||
block_number: Option<BlockId>, | ||
handle: Handle, | ||
rt: HandleOrRuntime, | ||
} | ||
|
||
impl<M: Middleware> EthersDB<M> { | ||
/// Create ethers db connector inputs are url and block on what we are basing our database (None for latest). | ||
/// | ||
/// Returns `None` if no tokio runtime is available or if the current runtime is a current-thread runtime. | ||
pub fn new(client: Arc<M>, block_number: Option<BlockId>) -> Option<Self> { | ||
let handle = match Handle::try_current() { | ||
let rt = match Handle::try_current() { | ||
Ok(handle) => match handle.runtime_flavor() { | ||
tokio::runtime::RuntimeFlavor::CurrentThread => return None, | ||
_ => handle, | ||
_ => HandleOrRuntime::Handle(handle), | ||
}, | ||
Err(_) => return None, | ||
}; | ||
|
@@ -31,29 +33,51 @@ impl<M: Middleware> EthersDB<M> { | |
Some(Self { | ||
client, | ||
block_number, | ||
handle, | ||
rt, | ||
}) | ||
} else { | ||
let mut instance = Self { | ||
client: client.clone(), | ||
client: client, | ||
block_number: None, | ||
handle, | ||
rt, | ||
}; | ||
instance.block_number = Some(BlockId::from( | ||
instance.block_on(client.get_block_number()).ok()?, | ||
instance.block_on(instance.client.get_block_number()).ok()?, | ||
)); | ||
Some(instance) | ||
} | ||
} | ||
|
||
// Create a new EthersDB instance, with a provider and a block (None for latest) and a runtime. | ||
// | ||
// Refer to [tokio::runtime::Builder] how to create a runtime if you are in synchronous world. | ||
// If you are already using something like [tokio::main], call EthersDB::new instead. | ||
pub fn with_runtime( | ||
client: Arc<M>, | ||
block_number: Option<BlockId>, | ||
runtime: Runtime, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so the caller is responsible for creating a new runtime? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sense, we can add a new function and document the behavior. |
||
) -> Option<Self> { | ||
let rt = HandleOrRuntime::Runtime(runtime); | ||
let mut instance = Self { | ||
client, | ||
block_number, | ||
rt, | ||
}; | ||
|
||
instance.block_number = Some(BlockId::from( | ||
instance.block_on(instance.client.get_block_number()).ok()?, | ||
)); | ||
Some(instance) | ||
} | ||
|
||
/// Internal utility function to call tokio feature and wait for output | ||
#[inline] | ||
fn block_on<F>(&self, f: F) -> F::Output | ||
where | ||
F: core::future::Future + Send, | ||
F::Output: Send, | ||
{ | ||
tokio::task::block_in_place(move || self.handle.block_on(f)) | ||
self.rt.block_on(f) | ||
} | ||
|
||
/// set block number on which upcoming queries will be based | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
use tokio::runtime::{Handle, Runtime}; | ||
|
||
// Hold a tokio runtime handle or full runtime | ||
#[derive(Debug)] | ||
pub(crate) enum HandleOrRuntime { | ||
Handle(Handle), | ||
Runtime(Runtime), | ||
} | ||
|
||
impl HandleOrRuntime { | ||
#[inline] | ||
pub(crate) fn block_on<F>(&self, f: F) -> F::Output | ||
where | ||
F: std::future::Future + Send, | ||
F::Output: Send, | ||
{ | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will panic if called within async execution context There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really make sense to clone
AlloyDB
orEthersDB
.Clone
was also added by me in a previous PR, but I recently found it was a mistake. Generally, it is due to:Provider
andMiddleware
are only safe to clone within the same runtime. If you send it across runtimes (or threads), many internal things and assumptions could break.To your use case which is similar to mine, my suggestion is:
Arc<Mutex<CacheDB<EthersDB>>>
across all threads. This avoids duplicate caching for each thread and can speed up your application overall.CacheDB
members (note they are public) except theEthersDB
and create a newEthersDB
instead. This generously is a manual "Clone" implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I have a challenge in using arc.
in order to create evm and then transact(), I must do lock. but, about all of my perf. overhead is related to these steps, so it convert my program in a literally single-threaded system.
do you have any suggestion for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
with_db
accepts borrows.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean
with_ref_db
? I want to update the cache in eachtransact
from multiple threads