-
Notifications
You must be signed in to change notification settings - Fork 125
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
Runtime update detector #635
Conversation
aeffb05
to
56fa096
Compare
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.
I really like the function detect_runtime_update
. That's great, thanks a lot! I'm not yet sure about the examples - maybe we can do better there.
Hash: DeserializeOwned + Copy + Decode, | ||
Client: Subscribe, | ||
{ | ||
subscription: EventSubscriptionFor<Client, Hash>, |
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.
Regarding the problem with the cancellation token: Subscription allows for "unsubscribe":
substrate-api-client/src/rpc/mod.rs
Lines 68 to 81 in 4d4f198
#[maybe_async::maybe_async(?Send)] | |
pub trait HandleSubscription<Notification: DeserializeOwned> { | |
/// Returns the next notification from the stream. | |
/// This may return `None` if the subscription has been terminated, | |
/// which may happen if the channel becomes full or is dropped. | |
/// | |
/// **Note:** This has an identical signature to the [`StreamExt::next`] | |
/// method (and delegates to that). Import [`StreamExt`] if you'd like | |
/// access to other stream combinator methods. | |
async fn next(&mut self) -> Option<Result<Notification>>; | |
/// Unsubscribe and consume the subscription. | |
async fn unsubscribe(self) -> Result<()>; | |
} |
Maybe I'm imagining things a little easier than they are, but: If the subscription is made public, or a respective function is implemented, one could unsubscribe, and break the loop by having .next_events_from_metadata()
return None
. That would resolve the problem with the sync version and make the cancellation token obsolete. Could that work?
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.
Hmm, it might work. I'm not quite sure what happens with waiting subscriptions when we unsubscribe. Anyway I think I would rather look at this in a follow up task.
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.
Agreed, lets investigate that in another issue
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.
I created #638 as a follow up
Hash: DeserializeOwned + Copy + Decode, | ||
Client: Subscribe, | ||
{ | ||
subscription: EventSubscriptionFor<Client, Hash>, |
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.
Agreed, lets investigate that in another issue
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.
Almost there! Just one thing left regarding the async example. Other than that, I think it's good to go 🥳 Thanks for your patience!
Co-authored-by: Bigna Härdi <bigna.h@hotmail.com>
Co-authored-by: Bigna Härdi <bigna.h@hotmail.com>
Co-authored-by: Bigna Härdi <bigna.h@hotmail.com>
Co-authored-by: Bigna Härdi <bigna.h@hotmail.com>
Co-authored-by: Bigna Härdi <bigna.h@hotmail.com>
aabc5c7
to
91863fa
Compare
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 for your patience with me! 💯
UpdateRuntime
was worth it&mut
reference ofApi
RuntimeUpdateDetector
but instead in the exampleRuntimeUpdateDetector
but I thought it is neccessary