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

Move away from async-channel #1389

Open
Tracked by #1383
jbesraa opened this issue Jan 23, 2025 · 12 comments · May be fixed by #1411
Open
Tracked by #1383

Move away from async-channel #1389

jbesraa opened this issue Jan 23, 2025 · 12 comments · May be fixed by #1411
Assignees
Labels
refactor Implies refactoring code roles Pertains to all roles roles-utils Utility logic for roles
Milestone

Comments

@jbesraa
Copy link
Contributor

jbesraa commented Jan 23, 2025

Motivation: We want to align the networking/communication layer to use only one external crate. Currently the code mixes tokio channels with async-channel channels. Removing the later should give us more confidence and improve the reliability of the code.
From async-channel only multi-producer-multi-consumer channels are used, when moving those to tokio one should reevaluate this assumption.

This task should be completed across all roles. The final result should have async-channel fully removed from roles workspace.

tokio channels https://tokio.rs/tokio/tutorial/channels

@jbesraa jbesraa mentioned this issue Jan 23, 2025
23 tasks
@plebhash plebhash added refactor Implies refactoring code roles-utils Utility logic for roles roles Pertains to all roles labels Jan 23, 2025
@plebhash plebhash added this to the 1.3.0 milestone Jan 28, 2025
@Shourya742 Shourya742 linked a pull request Jan 29, 2025 that will close this issue
@plebhash
Copy link
Collaborator

@Fi3 could you provide us some insight into what were the original motivations for mixing async_channel with tokio when designing the noise_connection_tokio module of network_helpers_sv2 crate?

plebhash added a commit to plebhash/stratum that referenced this issue Feb 1, 2025
plebhash added a commit to plebhash/stratum that referenced this issue Feb 1, 2025
plebhash added a commit to plebhash/stratum that referenced this issue Feb 2, 2025
@Fi3
Copy link
Collaborator

Fi3 commented Feb 3, 2025

I think cause to sketch up something fast we needed a mpmc channel (not present in tokio) but really don't remember.

@jbesraa
Copy link
Contributor Author

jbesraa commented Feb 3, 2025

aint the equivalent to mpmc is broadcast channel from tokio? also, in which cases did you need the mpmc channel?

@Fi3
Copy link
Collaborator

Fi3 commented Feb 3, 2025

just guessing I really don't remember have been a lot of time ago

@Fi3
Copy link
Collaborator

Fi3 commented Feb 3, 2025

btw if there is a reason is more something that have to do with getting a working example faster. Removed that dep from demand codebase has been one of the first thing that I did when I ported the code

@jbesraa
Copy link
Contributor Author

jbesraa commented Feb 3, 2025

Can you provide more info on which channels are using now?

@Fi3
Copy link
Collaborator

Fi3 commented Feb 3, 2025

@Fi3
Copy link
Collaborator

Fi3 commented Feb 3, 2025

I think that I use mpsc almost everywhere maybe some oneshoot in the pool

@Fi3
Copy link
Collaborator

Fi3 commented Feb 3, 2025

not sure if I answered your question?

@Fi3
Copy link
Collaborator

Fi3 commented Feb 3, 2025

yep in the pool I use the oneshot for handle authentication. in the cli there are several broadcast https://github.com/search?q=repo%3Ademand-open-source%2Fdemand-cli+broadcast&type=code

@jbesraa
Copy link
Contributor Author

jbesraa commented Feb 3, 2025

not sure if I answered your question?

Yup, thank you

@Shourya742
Copy link
Contributor

Shourya742 commented Feb 3, 2025

A small glimpse into my exploration of the issue:

Broadcast Channel (Tokio):

Sequence Diagram

Image

MPMC (async-channel):

Sequence Diagram

Image

In the case of an MPMC (Multi-Producer Multi-Consumer) channel, message retrieval follows a work-stealing mechanism, meaning that any available consumer can receive a message based on runtime scheduling, such as its position in the task queue. However, this behavior does not align with role-based processing, where specific consumers should receive specific messages. Which means only a single receiver is being used, otherwise each receiver might be getting different messages. Currently, our channel management in roles could be simplified. Instead of using an MPMC channel, we might only need to pass a single receiver to each role to access incoming messages, eliminating the need for multiple cloned receivers. The only reason we are using an MPMC channel right now is its clonable nature. Given how roles are currently structured—where a clones of receivers are shared across threads for access control—this approach might not be optimal. Instead, restructuring the way channels are passed across tasks could help simplify the design and potentially remove the need for an MPMC channel. From a high-level perspective, each role only requires a receiver to get messages from other role entities. This could be achieved using an MPSC channel instead of MPMC, since a role only needs one receiver instance. However, the main reason we currently use MPMC is to allow cloning the receiver.

plebhash added a commit to plebhash/stratum that referenced this issue Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Implies refactoring code roles Pertains to all roles roles-utils Utility logic for roles
Projects
Status: Todo 📝
Development

Successfully merging a pull request may close this issue.

4 participants