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

Refactoring light-base/src/json_rpc_service.rs #291

Closed
tomaka opened this issue Mar 14, 2023 · 4 comments
Closed

Refactoring light-base/src/json_rpc_service.rs #291

tomaka opened this issue Mar 14, 2023 · 4 comments

Comments

@tomaka
Copy link
Contributor

tomaka commented Mar 14, 2023

The code in json_rpc_service.rs is way too cumbersome and complicated, and more functionalities should be added to the request_subscriptions state machine in order to make it more simple.

Unfortunately, the logic of this code is inherently complicated. When a chainHead_unstable_body for example is called, we need to look up in the corresponding follow subscription to check whether the block is still pinned.

I can see two ways of making it work:

  • request_subscriptions could store, for each subscription, a task and a user data. When the user wants to unsubscribe, we ask the request_subscriptions to abort the task (after examining the user data, to be sure that the type of the subscription matches). The user data is behind a Mutex and would store, in the case of a follow subscription, the FollowSubscription struct that currently exists. This solution is pretty similar to what already is done in the code.

  • request_subscriptions could store, for each subscription, a task and a channel that can send messages to that task. The channel would be provided by the request_subscriptions rather than done manually in order to add some safety around locking Mutexes. When the user wants to unsubscribe, we send a message to the subscription in question which then politely shuts down. When the user calls for example chainHead_unstable_body, we send a message to the subscription in question as well, which then starts the body request subscriptions. The existing FollowSubscription would be a local variable in the task of the follow subscription.

The latter is more safe in terms of Mutex locking, as it would prevent all accidental deadlocks, but maybe also a bit more complicated.

@tomaka
Copy link
Contributor Author

tomaka commented Mar 14, 2023

cc #284 which was the first step

@tomaka
Copy link
Contributor Author

tomaka commented Mar 14, 2023

I'm leaning towards the second solution, as the lack of mutexes seems more clean to me.

@tomaka
Copy link
Contributor Author

tomaka commented Mar 21, 2023

The last step would be to refactor chain_head.rs to use a struct rather than one big async closure.

@tomaka
Copy link
Contributor Author

tomaka commented Jul 3, 2023

Done after #793

@tomaka tomaka closed this as completed Jul 3, 2023
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

No branches or pull requests

1 participant