Skip to content
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

Merged
merged 23 commits into from
Aug 4, 2023
Merged

Runtime update detector #635

merged 23 commits into from
Aug 4, 2023

Conversation

Niederb
Copy link
Contributor

@Niederb Niederb commented Jul 27, 2023

  • Adds a class that allows to wait for runtime updates
  • Not sure if extracting the trait UpdateRuntime was worth it
  • The design and features are somewhat limited by the fact that we don't want to use a &mut reference of Api
  • I implemented two examples: sync and async. I tried with having only one but the examples ended up somewhat confusing. So I separated them.
  • Canceling the future is not handled inside of RuntimeUpdateDetector but instead in the example
    • The could not find a simple way of doing it without relying on Tokio and using Tokio would restrict the runtime choice of the user
  • I'm not super happy about the implementation for canceling the RuntimeUpdateDetector but I thought it is neccessary
    • Note that it is not bulletproof for the sync case. This is due to the fact that getting the events is blocking with no option of canceling the wait. So if no more events are received the call is still stuck

@Niederb Niederb changed the title Runtime update Runtime update detector Jul 27, 2023
@Niederb Niederb marked this pull request as ready for review July 27, 2023 11:49
Copy link
Contributor

@haerdib haerdib left a 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.

examples/examples/runtime_update_sync.rs Outdated Show resolved Hide resolved
src/api/rpc_api/runtime_update.rs Outdated Show resolved Hide resolved
src/api/rpc_api/runtime_update.rs Outdated Show resolved Hide resolved
src/api/rpc_api/runtime_update.rs Outdated Show resolved Hide resolved
examples/examples/runtime_update_async.rs Show resolved Hide resolved
node-api/src/events/event_details.rs Outdated Show resolved Hide resolved
examples/examples/runtime_update_async.rs Show resolved Hide resolved
src/api/rpc_api/runtime_update.rs Show resolved Hide resolved
Hash: DeserializeOwned + Copy + Decode,
Client: Subscribe,
{
subscription: EventSubscriptionFor<Client, Hash>,
Copy link
Contributor

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":

#[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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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

@Niederb Niederb requested a review from haerdib July 28, 2023 14:07
examples/examples/runtime_update_async.rs Show resolved Hide resolved
examples/examples/runtime_update_sync.rs Outdated Show resolved Hide resolved
examples/examples/runtime_update_sync.rs Outdated Show resolved Hide resolved
Hash: DeserializeOwned + Copy + Decode,
Client: Subscribe,
{
subscription: EventSubscriptionFor<Client, Hash>,
Copy link
Contributor

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

Copy link
Contributor

@haerdib haerdib left a 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!

examples/examples/runtime_update_async.rs Outdated Show resolved Hide resolved
examples/examples/runtime_update_async.rs Show resolved Hide resolved
Copy link
Contributor

@haerdib haerdib left a 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! 💯

examples/examples/runtime_update_async.rs Show resolved Hide resolved
@Niederb Niederb merged commit 2ffb07e into master Aug 4, 2023
40 checks passed
@Niederb Niederb deleted the tn/runtime-upgrade-2 branch August 4, 2023 06:14
@Niederb Niederb linked an issue Aug 4, 2023 that may be closed by this pull request
@haerdib haerdib added the F8-newfeature Introduces a new feature label Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E1-breaksnothing F8-newfeature Introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce easy runtime updates (loop?)
2 participants