MatrixAI / Polykey

Polykey Core Library
https://polykey.com
GNU General Public License v3.0
29 stars 4 forks source link

Lazy Creation of WebSocketClient within PolykeyClient #771

Open amydevs opened 1 month ago

amydevs commented 1 month ago

Specification

After experimenting with PolykeyClient in Polykey Network Status. I've realized that the error handling is really tedious.

The main issues happen when a connection fails, the error you will receive from the PolykeyClient will be inconsistent depending on the point at which the code throws during the the stopping process.

This is because the Stopping signal propagation of WebSocketClient to the PolykeyClient is entirely asynchronous through EventListeners. This makes for a huge pain when attempting connection retries like so:

https://github.com/MatrixAI/Polykey-Network-Status/blob/staging/src/plugins/polykeyClientPlugin.ts

This initially gave me the impression that error handling with RPCClient + WebSocketClient was really messy.

But after playing around with it in React, I managed to come up with a small React context provider component that handles it all really elegantly: https://github.com/MatrixAI/Polykey-Enterprise/blob/feature-ws/src/app/contexts/RPCProvider.tsx

The gist is that instead of the lifecycle of PolykeyClient being flakily tied to WebSocketClient, the PolykeyClient will instead always be started, creating WebSocketConnections lazily when an RPCClient call is being attempted.

This has several benefits:

Implementation

A lazy param will be simply added to the PolykeyClient constructor. There will be no API changes.

There WILL however be behaviour changes that need to be documented:

Additional context

Tasks

  1. Add lazy option to PolykeyClient
linear[bot] commented 1 month ago

ENG-366 Lazy Creation of WebSocketClient within PolykeyClient

tegefaulkes commented 3 weeks ago

There is some stuff the be worked out with differences in behaviour. With the network state being lazily created we can end up with not actually active networking state while we're runing. This means getters for host and port will return undefined. This changes the API somewhat.

@amydevs Mentioned we can branch the behaviour into two separate classes to avoid the behaviour conflict. but this is something to be addressed later.