Asyncify BareCryptoStore trait #5940
Conversation
rakanalh
added
the
A3-in_progress
rakanalh
requested review from
kianenigma,
mxinden,
NikVolf,
pepyakin and
tomusdrw
as code owners
rakanalh
added
A0-please_review
| pub trait BareCryptoStore: Send + Sync { | ||
| /// Returns all sr25519 public keys for the given key type. | ||
| fn sr25519_public_keys(&self, id: KeyTypeId) -> Vec<sr25519::Public>; | ||
| async fn sr25519_public_keys(&self, id: KeyTypeId) -> Vec<sr25519::Public>; |
There was a problem hiding this comment.
Note that all methods here will most likely end up returning a Result so that we can handle potential remote signer issues (unless we want them to fall back to some defaults).
| @@ -64,8 +67,9 @@ impl KeyStore { | |||
| } | |||
|
|
|||
| #[cfg(feature = "std")] | |||
| #[async_trait] | |||
| impl crate::traits::BareCryptoStore for KeyStore { | |||
There was a problem hiding this comment.
Might be good to simply introduce a blocking wrapper for BareCryptoStore instead of sprinkling block_on everywhere. You can do that without introducing any new structs, just adding an extra trait that maintains current API something like this:
pub trait BlockingBareCryptoStore {
fn keys() -> Vec<...>
}
impl<T: BareCryptoStore> BlockingBareCryptoStore for T {
fn keys() -> Vec<...> { block_on(BareCryptoStore::keys(self)) }
}Unless we plan to asyncify more parts of the code in the future, in that case a blockign variant is not worth it.
tomusdrw
added
A6-mustntgrumble
and removed
A0-please_review
| ) | ||
| .map_err(|_| Error::Signing)?; | ||
| let keystore = key_store.read(); | ||
| let signatures = block_on(keystore.sign_with_all( |
There was a problem hiding this comment.
I don't think blocking on async logic is an option. Instead this should properly pass a context and return Poll::Pending in case the future is not yet ready.
Why not use block_on:
Logic further below might expect to be called in a preemptive non-blocking fashion. Making that assumption in a blocking environment is problematic.
Say e.g. one executes on a single hardware thread. The logic below needs to wait for something, thus returns Poll::Pending. Given that one uses block_on here the thread is stalled and thus the dependency (the thing the logic below is waiting for) never gets any compute time. Thus one has a deadlock.
There was a problem hiding this comment.
Rather than passing a context, I think a better solution is to turn publish_ext_addresses into an async function and adjusting poll to poll it progressively.
That involves extracting publish_ext_addresses into a freestanding function and passing copies of client, network, etc. but I don't think that's a problem.
| @@ -105,7 +105,7 @@ impl<P, Client> AuthorApi<TxHash<P>, BlockHash<P>> for Author<P, Client> | |||
|
|
|||
| let key_type = key_type.as_str().try_into().map_err(|_| Error::BadKeyType)?; | |||
| let mut keystore = self.keystore.write(); | |||
| keystore.insert_unknown(key_type, &suri, &public[..]) | |||
| block_on(keystore.insert_unknown(key_type, &suri, &public[..])) | |||
There was a problem hiding this comment.
We could change the RPC to return a future instead (i.e. asyncify it)
| @@ -28,6 +28,9 @@ | |||
|
|
|||
| use sp_std::vec::Vec; | |||
|
|
|||
| #[cfg(feature = "std")] | |||
| use futures::executor::block_on; | |||
There was a problem hiding this comment.
Is it ok to totally block a runtime execution on potentially some long-duration I/O?
|
Closing this and putting it on hold to implement a different approach. |
This is part of #4689.
Resolves #5931