Add autoconnect to ScProvider

[wirednkod]

Resolves issue mentioned in substrate connect repo - issues #1243 and #1141

[tomaka]

The objective is to add an equivalent of this block of code:

api/packages/rpc-provider/src/ws/index.ts

Lines 450 to 456 in 12750bc

	if (this.#autoConnectMs > 0) {
	setTimeout((): void => {
	this.connectWithRetry().catch(() => {
	// does not throw
	});
	}, this.#autoConnectMs);
	}

We want to automatically reconnect when we disconnect.

[tomaka]

So it's not that simple. The problem to solve is not what to do when the health checker says that we're not healthy. The problem to solve is what to do when the connection to the extension dies. In other words, you should try to reconnect when an error event happens. The problem is that there's no way right now from the API of the connect package to detect when the connection to the extension has died, other than the fact that the Chain.sendJsonRpc and Chain.remove functions throw an exception.

[wirednkod]

So it's not that simple. The problem to solve is not what to do when the health checker says that we're not healthy. The problem to solve is what to do when the connection to the extension dies. In other words, you should try to reconnect when an error event happens. The problem is that there's no way right now from the API of the connect package to detect when the connection to the extension has died, other than the fact that the Chain.sendJsonRpc and Chain.remove functions throw an exception.

Im not sure why you refer to the extension. At the moment, When laptop goes to sleep and then resumes, the current code was cleaning up all subscriptions due to the emit of disconnect. The health checker continues to show that we are healthy, but since the subscription is cleared - there is no way to communicate something back to the App.
What is solved in this PR, is to avoid killing subscriptions when autoconnect is set to true (default and when a manual disconnect does not take place).
This way when the computer resumes, the subscription still exists and receives all the messages.

[jacogr]

It look sane. Happy to approve even if it only partly moves in the right direction.

[tomaka]

This PR "seems" wrong to me. It would mean that the provider can report a disconnected event, then later a connected event, without the active subscriptions being killed.

I can't strictly say that this is wrong because it depends on the small details of the API of the Provider interface, and these small details aren't written anywhere. But it definitely feels wrong to me.

[wirednkod]

The way I approached it, is that the subscription should be cleaned only if disconnect is called. Everything else (emit connect - emit disconnect - emit connect) seems to me as a network failure (intentional through sleep mode or not), and IMO subscriptions should not be cleared).

[jacogr]

So the events handling would depend fully on the underlying layer. For instance in WS, when you get a disconnect, it would mean that the underlying transport is gone and dead, you will never receive anything on it again. In the WsProvider, after a network layer disconnect/re-connect, all subs that were existing re-created to ensure continuance. (Just continuing with the current ones will never have any data)

So in this case, if a disconnect/reconnect means that the existing subs are still ongoing, it would be fine. If not, it would mean you have subscriptions that won't get any data back.

[tomaka]

The entire and only reason why we have this health checker system and why we generate disconnected and connected events is precisely to prevent PolkadotJS from querying information at certain moments.

When we generate a "disconnected" event, we precisely mean "DO NOT USE THE CLIENT RIGHT NOW /!\ /!"

It feels extremely surprising to me that subscriptions would stay alive during this mode.
We should, IMO, do it the same way as the WsProvider and treat "disconnected" as "the client is completely unaccessible", rather than in a mode where everything is unaccessible but subscriptions stay alive.

[tomaka]

The ScProvider is currently behaving the same way as the WsProvider, and this PR makes the ScProvider more tolerant in an attempt to fix a problem somewhere.

To me this indicates that this is not the correct fix. If things work fine with the WsProvider, and they do, then they should work fine with the ScProvider as well. And if necessary we should instead try to make the ScProvider behave more similarly to the WsProvider, rather than differently.

[tomaka]

Ok, read more, and the WsProvider re-subscribes:

api/packages/rpc-provider/src/ws/index.ts

Line 540 in 96a23bd

#resubscribe = (): void => {

I don't really understand why this is done in the WsProvider rather than a different layer, but this is what we should do in the ScProvider.

Stale review, changes made since.

[jacogr]

One small comment, looks sane otherwise.

It could make sense, now that there are 2 providers that require the same functionality, to extract resubscriptions - not for this PR, it would need investigation (could probably use an issue).

[wirednkod]

could probably use an issue

As per your request: #5228

[jacogr]

Thank you.

[tomaka]

It's missing a this.#resubscribeMethods.delete when you unsubscribe

[tomaka]

It's also not clear to me why resubscribeMethods needs to be a different map than subscriptions

[wirednkod]

It's also not clear to me why resubscribeMethods needs to be a different map than subscriptions

Because subscriptions upon disconnect are cleared. Thats the initial source of the problem I described before. When disconnect occurs the cleanup empties the Map and thus reconnect cannot work.

I'll fix the .delete on a new PR.

[polkadot-js-bot]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.