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

Integrate TaskManager in reth main #1036

Closed
mattsse opened this issue Jan 25, 2023 · 8 comments · Fixed by #1292
Closed

Integrate TaskManager in reth main #1036

mattsse opened this issue Jan 25, 2023 · 8 comments · Fixed by #1292
Labels
A-cli Related to the reth CLI C-enhancement New feature or request S-blocked This cannot more forward until something else changes

Comments

@mattsse
Copy link
Collaborator

mattsse commented Jan 25, 2023

Describe the feature

The tasks crate provides TaskManager and TaskExecutor

The TaskExecutor is used for spawning new tasks (can be critical tasks, e.g. network tasks, or regular, like p2p session tasks)

The TaskManager is notified when a critical task has panicked, which can be used as an exit condition.

It's intended that the TaskManager is the main entry point when launching the node.

For example, this will run until a critical task has panicked

let panicked = manager.next().await;

// this will terminate _all_ tasks spawned via an executor
drop(manager)

The shutdown process needs some additional work, I guess ideally we want to listen for ctrl-c -> shutdown tasks

https://github.com/paradigmxyz/reth/blob/main/crates/tasks/src/lib.rs

Additional context

ref #807

@mattsse mattsse added C-enhancement New feature or request S-needs-triage This issue needs to be labelled A-cli Related to the reth CLI labels Jan 25, 2023
@literallymarvellous
Copy link
Contributor

I would like to take this.

Currently, cargo run --bin reth node is returning a Could not load config error.

@mattsse mattsse added the S-blocked This cannot more forward until something else changes label Jan 27, 2023
@mattsse
Copy link
Collaborator Author

mattsse commented Jan 27, 2023

appreciate it, but this would do more harm atm, since there's a bunch of work underway for the CLI

blocked by
#1017
#963

If you want to take on something that's slightly more challenging but can be done incrementally #1037 would be a good start

I believe the config error is due to #1046

@literallymarvellous
Copy link
Contributor

Alright, I'll take 1037.

@onbjerg onbjerg removed S-needs-triage This issue needs to be labelled S-blocked This cannot more forward until something else changes labels Jan 27, 2023
@onbjerg
Copy link
Collaborator

onbjerg commented Jan 28, 2023

Should no longer be blocked. One note though is that I want this to be optional, i.e. we should try to not make it a requirement to use the task manager for crate consumers

@literallymarvellous
Copy link
Contributor

Alright, will still like to take this.

Concerning making TaskManager optional, how would that work? Does reth main run without the task manager by default, maybe have a cli flag for the task manager functionality?

@mattsse
Copy link
Collaborator Author

mattsse commented Jan 28, 2023

same as:

/// The executor to use for spawning tasks.
pub executor: Option<TaskExecutor>,

I think we need a helper type for Option<TaskExecutor>
perhaps MaybeTaskManager(Option<TaskExecutor>)

@onbjerg onbjerg mentioned this issue Jan 28, 2023
11 tasks
@onbjerg
Copy link
Collaborator

onbjerg commented Jan 28, 2023

Blocked by #1079 now, sorry

maybe have a cli flag for the task manager functionality?

No, we should always use the task manager in the CLI. The reason it should be optional is in the case where you are using the crates yourself and you would like to use your own runtime. We should strive to not use tokio::task and tokio::spawn at all, if possible

Ideally we would simply get the things we can spawn, and then spawn them in the CLI. E.g. instead of start_network existing, we should probably just have it return the tasks we need to spawn (NetworkManager etc) and then spawn them ourselves

@onbjerg onbjerg added the S-blocked This cannot more forward until something else changes label Jan 28, 2023
@literallymarvellous
Copy link
Contributor

literallymarvellous commented Jan 28, 2023

oh, okay. I get it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Related to the reth CLI C-enhancement New feature or request S-blocked This cannot more forward until something else changes
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants