Stabilise the `unstable-reconnecting-rpc-client`

jsdw commented 2 months ago

We'd like to stabilise the automatic reconnecting logic and make it the default behaviour.

I think that this requires two things:

Making sure that the reconnecting logic in Subxt is tested. This means testing the Backend impls, but also some of the structs returned that hande eg storage streams and making sure that they all work as expected. Let's unit test what we can, and if needbe we can create a mock RpcClient implementation and see how things respond to that returning an error or disconnecting.
Niklas had the idea to take what we need from https://crates.io/crates/reconnecting-jsonrpsee-ws-client and inline it into Subxt, since I think that crate exposes a bunch of features we don't need given that we handle all of the reconnect logic in Subxt.

Once these are done and we have some confidence that it works, we can make this the default and remove the feature flag entirely. Maybe we should continue exposing the "not reconnecting" jsonrpsee client too in case people want to fall back to the old behaviour or handle reconnecting in some other way.

niklasad1 commented 1 month ago

Yes, I can confirm that this is the roadmap

pkhry commented 3 weeks ago

I've encountered some issues with testing The Backend trait and its implementations.

Explanation

The Backend impl requires a RpcClient implementation which would connect to the Server. But the problem is that RpcClient is using serde_json::from_strin request and RpcSubscription. RpcClient also expects RpcClientT to return serde_json::raw::RawValue. So to test the Backend implementation we need to either mock the RpcClient or the server implementations.

Mocking the RpcClient

Using concrete implementation of RpcClientT and then passing it to the RpcClient. The problem is that we don't have Serialize instances defined for many data types inside subxt that we expect to return inside the responses from RpcClientT. Ways to circumvent this without defining Serialize for every type we need are:

cfg_attr conditional attribute
just use serde_json::value::Value/json!() and build data types by hand to be returned in each case by the RpcClientT.
Try to decouple RpcClient from concrete wire format, to be able to pass identity values of things we expect to get back from the RpcClientT -> this seems completetely useless as we use JSON-RPC.
Mocking the Server

Seems like an overkill to me as this will require us to start up and kill a test server for every test and any additional wins in testing would be for RpcClientT implementations, but this feels like testing internals of the libraries we use.

In short:

i'd prefer to go with testing the Backends with mocked RpcClient and just using cfg_attr for deriving serialization during testing and to not go the way of testing the Rpc clients impls along the Backend impl. So i feel like for unstable-reconnecting-client it might be just enough to test the fact that it does indeed reconnects when node goes down inside integration-tests

unstable-reconnecting-rpc-client tests already done:

I've plugged into integration test-suite in #1711 and added a reconnection to one of the tests there.

niklasad1 commented 3 weeks ago

Thanks for the writeup and indeed mocking the RpcClient sounds good enough for this if it's possible to test reconnect on block subscriptions and similar on but it's important to test both the legacy and unstable backend because they differ a bit.

As we already discussed, I'm completely fine to add Serialize/Deserialize for the types in the backend as a first step to see how it will look. It should be quite easy to feature-gate those if needed but would be good to see how it will look like and hard to tell for me right now without any code...

Thus, mocked RPC client test in combination with the integration test in #1711, seems like a reasonable acceptance level.

I wrote some unit tests for the finalized block subscriptions for the unstable backend that it's probably the only tests we got so far, https://github.com/paritytech/subxt/blob/master/subxt/src/backend/unstable/follow_stream_driver.rs#L650-#L745.

FYI, the actual behavior between the unstable backend and legacy differs where we don't keep track of missed blocks for the legacy one i.e, we just emit a notification on the subscription that the connection was lost whereas for the unstable we actually keep track of whether a block was missed or not..

jsdw commented 3 weeks ago

I agree with the above too offhand! Mocking the RpcClientT and then either using hand written JSON or (if that is a pain) adding Serailize attrs behind the test attr makes sense to me!

paritytech / subxt