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

JSON-RPC service is still too complicated #786

Closed
tomaka opened this issue Jun 20, 2023 · 1 comment · Fixed by #793
Closed

JSON-RPC service is still too complicated #786

tomaka opened this issue Jun 20, 2023 · 1 comment · Fixed by #793

Comments

@tomaka
Copy link
Contributor

tomaka commented Jun 20, 2023

cc #291

I think it needs another refactor to get rid of requests_responses entirely, as this thing is too complicated.

I think that everything should be done through lightweight tasks sending messages to each other, which is a relatively simple model to reason about. There shouldn't be any Arc (except maybe to share read-only fields) or Mutex, instead the tasks should hold local variables.

For each JSON-RPC client (assuming the context of a full node where there are multiple clients), there should be a task dedicated to it. This task reads and writes the socket, decoding/encoding WebSocket frames. It should hold:

  • The socket.
  • A bounded queue of strings to write on the socket.
  • A sender and receiver to the task below. This sender and receiver can be back-pressured.

For each JSON-RPC client, there should also be a task that serves as the reunion point of all the requests, responses and notifications, and dedicated to ordering/skipping the messages. It receives the requests, parses them, and dispatches them to other tasks or spawns a notifications task. It receives the responses and notifications, and processes them immediately by acting as the synchronization point of everything specific to a client. It should hold:

  • A sender and receive to the socket-dedicated task (see above). This sender and receiver can be back-pressured.
  • The number of active subscriptions, to check against the limit.
  • The subscription ids allocator.
  • A receiver for each request that has been dispatched to other tasks. These receivers should only be back-pressured for CPU reasons.
  • A receiver for each notification. These receivers should only be back-pressured for CPU reasons.
  • Senders to the requests-processing tasks. These senders can be back-pressured.

There should be a task dedicated to processing serially the simply "getter" JSON-RPC requests, such as chain_getFinalizedHead, system_chain, system_localPeerId, etc. whose processing simply consists in reading a variable somewhere.

In the light client, there should be a task dedicated to cache accesses. It receives requests that require accessing the cache, does the cache access, then dispatches the request+cache to another task.

There should be a certain number of tasks dedicated to the requests that need networking access (e.g. state requests).

For some types of notifications, there should be a task per active notification.
For other types of notifications, there should be a single task for the whole type, which receives new senders.

@tomaka
Copy link
Contributor Author

tomaka commented Jun 20, 2023

I think that this is the best approach. The mistake of the current code is to try isolate too much the code of each request/subscription, rather than spread the logic of the methods throughout the entire JSON-RPC service.

The only difficulty that I see with this new design is how to write code that could be shared between the full node and light client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant