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

[CX_CLEANUP] Create new Task Abstraction async_broadcast Channels #2500

Closed
wants to merge 10 commits into from

Conversation

bfish713
Copy link
Collaborator

@bfish713 bfish713 commented Feb 1, 2024

Closes #2403

This PR:

Creates a few async primitives for use in HotShot. The next PR integrates them.

The first set of things are Task primatives. This is meant to replace the "hotshot-task" crate currently in the code ("crates/task"). The tasks are based on async_broadcast channels. These channels provide us with very nice set of properties. First they are MPMC (Multi Producer, Multi Consumer) which we need, and second they have the nice property that clones of the Receiver will receive all messages the original receiver receives (I've gotten very good at spelling receiver haha). This means that spawning a sub task is now possible. If the spawned task takes a Receiver clone from the parent it will only "miss" the event which triggered the spawning, which can easily be accounted.

The tasks work with a simple wait for events loop. In the loop we check for shutdown conditions, do filtering and handle the event. Using just channels and awaiting means no more need for tasks to sleep/yeild to allow other tasks to run. The async executor and the channel itself will properly awake tasks to run when things are ready.

The main task primitives are:

  • Task - This is a simple version of the old task. It simply loops waiting for events and handles them until it reaches a shutdown condition
  • TestTask - Similar to Task but it receives messages from a bunch of receivers. The idea is to use this for testing tasks, like the OverallSafetyTask to receive messages from all of the nodes in the test. It contains within it a Task as well which is simply to hold the event receiver, registry, and state. The event receiver should be for receiving internal events, so for testing tasks this will be GlobalTestEvents.
  • TaskState trait - This trait is for defining the behavior of the tasks. In our code the main tasks are pretty simple, they handle events until they shutdown. We allow for event filtering and there is a hook to cleanup after a shutdown condition. This is for saving some special state if a Shutdown event is received. All the main logic task will implement this trait rather than passing in a EventHandler to some registry.
  • TestTaskState trait - extends the TaskState trait with the addition of handling messages. Messages are events but from outside tasks (outside isn't the best word but I mean: from not other testing things). In our case this would be events from the main tasks.

I also created some "Dependency" abstractions. These abstractions are for encapsulating the idea of waiting for some preconditions to be met, then executing something which uses the results of the preconditions. And example of preconditions might be waiting for a DACRecv, QCFormed, and vid data received events, and the execution would be to use all 3 of those events to form and send a QuorumProposal
The main primitives are:

  • Dependency trait - This just provides an interface for something that can be awaited and return some data of type T. It allows dependencies to be compostable.
  • AndDependency - This is a dependency which collects many futures and is complete when all the futures complete. It's intended to combine multiple other dependencies. Essentially futures::future::join_all but for specifically dependencies
  • OrDependency - Similar to AndDependency, but completes when any of it's futures succeeds and returns just the completed dependency result. Essentially futures::future::select_all but for dependencies.
  • CombinedDependency trait - is a nice way to combine dependencies into and or or versions. See the tests for examples
  • EventDependency - A dependency that completes when it gets a specific event. Takes a closure which returns whether an event is a match. This is similar to TaskState::filter_event but is a closure so you can do things like look for a proposal for a given view (which will dynamically change each view).
  • DependencyTask - A task which awaits on some dependency then runs so code which takes the results of it's dependency as input.

In my next PR I'm using the Task part of this extensively so I'm somewhat confident that it will provide us with what we need. The dependency stuff can probably be improved and made more ergonomic as well. I'm happy to here any feedback on the interface, and I'm sure we'll have to iterate on this as we actually use it in implementation.

This PR does not:

This PR does not add or change any functionality, it's simply to allow review for the new types.

Key places to review:

All the added coded. For examples see my next PR #2493

Copy link
Member

@elliedavidson elliedavidson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not approving because I think we need to enable lints. Otherwise I approve.

crates/new_task/src/dependency.rs Outdated Show resolved Hide resolved
crates/new_task/src/dependency.rs Show resolved Hide resolved
crates/new_task/src/dependency_task.rs Outdated Show resolved Hide resolved
crates/new_task/src/task.rs Show resolved Hide resolved
Copy link
Contributor

@QuentinI QuentinI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more comments from my end, looks awesome
I'll leave the approval to Ellie

@bfish713 bfish713 requested review from QuentinI and removed request for QuentinI February 9, 2024 04:49
@bfish713 bfish713 closed this Feb 10, 2024
@bfish713
Copy link
Collaborator Author

Closed because #2493 incorporates these changes

@bfish713 bfish713 deleted the bf/task-abstraction branch March 13, 2024 15:17
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 this pull request may close these issues.

[CX-CLEANUP] - Create a Task for Collecting Dependencies
3 participants