-
Notifications
You must be signed in to change notification settings - Fork 240
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
feat: add reconnecting-rpc-client
#1396
Conversation
reconnecting-rpc-client
reconnecting-rpc-client
@@ -101,3 +101,94 @@ impl<T: RpcClientT> RpcClientT for Box<T> { | |||
(**self).subscribe_raw(sub, params, unsub) | |||
} | |||
} | |||
|
|||
#[cfg(feature = "reconnecting-rpc-client")] | |||
impl RpcClientT for reconnecting_jsonrpsee_ws_client::Client { |
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 would probably place this under a src/client/reconnecting_client
module. Maybe within a ReconnectingClientRpC(reconnecting_jsonrpsee_ws_client::Client)
.
Ideally we would have a ReconnectingClient<T: Config>
that has a ReconnectingClient::builder
method and implements OnlineClientT
and OfflineClientT
traits.
Then we could also expose on the ReconnectingClient
a reconnect_count
method; and other methods of interest.
This would tidy up a bit the user API. Although the tricky part would be that we'll duplicate the reconnecting_jsonrpsee_ws_client::Client::builder
methods on the ReconnectingClient::builder
.
Considering this is under a feature-flag at the moment, I'd leave that refactoring as a follow-up as probably would need some extra tought.
What do you think?
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.
My gut feeling is that for now it should just be a standalone optional RPC client that you can use to instantiate the existing OnlineClient
, but at some point when we've added more special reconnect logic in places etc we can just use it by default in the current OnlineClient
instead of the non-reconnecting-client :)
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 agree with the separate module though; something like src/backend/rpc/reconnecting_jsonrpsee_impl.rs
maybe to sit next to the current jsonrpsee_impl.rs
?
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.
This looks good to me! One small nit about moving the RpcClientT
implementation for the reconnecting client to a dedicated module. In the future, we could introduce a more complete API (some initial thoughts #1396 (comment))
} | ||
// In this example we just ignore when reconnections occurs | ||
// but it's technically possible that we can lose | ||
// messages on the subscription such as `InFinalizedBlock` |
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 like this example overall, but when submitting a tx it's particularly tricky to handle as you will lose the subscription anyway won't you and won't get any new messages after reconnecting?
So perhaps a better (and simpler, anyway) example is just subscribing to finalized blocks?
I can't remember how your reconnecting client handles subscriptions etc; does it automatically re-subscribe behind the scenes? That area is an interesting one because on one hand, it might be better if the reconnecting client just reconnects but doesn't try to re-establish anything (so that the Subxt backend's can choose how best to restart stuff), but on the other hand having the client do it is simpler at least (but I fear may not always do what you want). Ultimately we'd need to agree on a "standard" for what is expected from a reconnecting client when it's used in Subxt
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 can't remember how your reconnecting client handles subscriptions etc; does it automatically re-subscribe behind the scenes?
Yes, it re-subscribes to subscriptions and re-transmits pending method calls.
It's just a PITA if one have plenty of storage subscriptions opened and manually have to re-subscribe to them.
For rpc-v2 then it's a different story but not really thought of in this 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.
Let's just make it optional and folks can try it out and complain what's bad :)
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.
Yeahh for now it's behind a feature flag so we can experiment!
My gut feeling is that before it becomes "stable" in Subxt, we'll prob end up wanting the Backend impls to decide when/how to resubscribe etc themselves (eg maybe for TXs we don't try re-sending but for following new/final blocks we just restart it or whatever
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.
Alex had a few cool ideas about distinguish between "stateful (submit_and_watch)" and "stateless (subscribe_storage)" subscriptions but still I think this reconnecting rpc client won't really work that well with the rpc-v2 spec.
One would need to call a few methods for the chainHead_unstable_follow
to make emit interesting stuff..
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.
So perhaps a better (and simpler, anyway) example is just subscribing to finalized blocks?
I want to show how to ignore "DisconnectWillReconnect" and continue if a reconnection occurs. If you just call wait_for_finalized_success
it will quit with that error but we could wait_for_finalized_success_and_reconnect
... I guess.
Added a few comments in the example now hopefully clearer now.
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 think this conversation is good start to open another issue how to deal with it reconnections in subxt properly especially with the rpc v2 spec in mind which is particular tricky.
In general, I think just reconnect is probably the way to go and not re-subscribe and so on but we let's try my crate first and collect some feedback what's the best way forward :)
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 want to show how to ignore "DisconnectWillReconnect" and continue if a reconnection occurs. If you just call wait_for_finalized_success it will quit with that error but we could wait_for_finalized_success_and_reconnect... I guess.
What I mean is just that it's less code to subscribe to finalied blocks and then show how you can ignore the DisconnectedWillReconnect error. Probably closer to being a valid thing that we can handle nicely too (we can just start the subscription up again as your client does atm anyway)
With transactions we probably(?) don't want to re-submit automatically so showing it as an example sortof hints at a behaviour that I don't think we'll keep, but anyway it's also just more effort building the tx etc when the point of the example is just handling the DisconnectWillReconnect error :)
In general, I think just reconnect is probably the way to go and not re-subscribe and so on but we let's try my crate first and collect some feedback what's the best way forward :)
Yup I agree; I'm happy with this for now since it's behind a feature flag, but before we stabilise it I thnk we should make the RPC client not try to do anything clever and only reconnect. Instead we can make the backend impls decide how to manage disconnects in followup PRs
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.
What I mean is just that it's less code to subscribe to finalied blocks and then show how you can ignore the DisconnectedWillReconnect error. Probably closer to being a valid thing that we can handle nicely too (we can just start the subscription up again as your client does atm anyway)
With transactions we probably(?) don't want to re-submit automatically so showing it as an example sortof hints at a behaviour that I don't think we'll keep, but anyway it's also just more effort building the tx etc when the point of the example is just handling the DisconnectWillReconnect error :)
Got it, makes sense
subxt/Cargo.toml
Outdated
@@ -42,6 +42,9 @@ web = [ | |||
"instant/wasm-bindgen" | |||
] | |||
|
|||
# Enable this to use the reconnecting rpc client | |||
reconnecting-rpc-client = ["dep:reconnecting-jsonrpsee-ws-client"] |
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.
Should we prefix with "unstable" for now until we've settled on the semantics of it (ie probably that the client will reconnect butnot auto resubscribe etc) and handled the DisconnectedWillReconnect?
Mostly LGTM; prooobably we should stick "unstable-" in front of the feature flag so that we can keep playing with the semantics without caring about breaking/changing behaviour between releases, and I still think we could simplify the example to subscribe to finalized blocks instead to show the current reconnect behaviour :) It also occurs to me that the example isn't referenced in the book anywhere, but for now I don't mind that; maybe we'll hold off until we stabilise it more! |
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.
Looks great to me; nice one!
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.
Looks good to me! Nice job here!
For some reason, I keep thinking of having a bit more control over the RetryPolicy.
As a first step, having a global policy makes a lot of sense.
We could look in the future at overriding the policy at a local level of subscriptions / methods. For example, having an API like:
let rpc = Client::builder()
// This is the global default policy, unless specified elsewhere this is used.
.retry_policy(ExponentialBackoff::from_millis(100).max_delay(Duration::from_secs(10)))
// For the `tx_submitAndWatch` we don't attempt at resubmitting the tx again; and let the user handle this if wanted
.local_policy(subxt::rpc::Method::TransactionSubmitAndWatch, Policy::DoNotRetry)
Nothing to worry about in this PR, nicely done again!
This PR adds a
reconnecting-rpc-client
behind a feature flag--feature unstable-reconnecting-rpc-client
and re-exports it from subxt.The reason why it's added in subxt is for ease-of-use because of orphan-rules etc for anyone that will use it i.e, everyone would need to implement the RpcClientT but won't work without a new type pattern which makes the example much more complicated.
Resolves #1190, resolves #249