General task architecture refactor #3133
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ss-es
commented
May 18, 2024
Comment on lines
-108
to
-137
| impl< | ||
| TYPES: NodeType, | ||
| VOTE: Vote<TYPES> | ||
| + AggregatableVote<TYPES, VOTE, CERT> | ||
| + std::marker::Send | ||
| + std::marker::Sync | ||
| + 'static, | ||
| CERT: Certificate<TYPES, Voteable = VOTE::Commitment> | ||
| + Debug | ||
| + std::marker::Send | ||
| + std::marker::Sync | ||
| + 'static, | ||
| > TaskState for VoteCollectionTaskState<TYPES, VOTE, CERT> | ||
| where | ||
| VoteCollectionTaskState<TYPES, VOTE, CERT>: HandleVoteEvent<TYPES, VOTE, CERT>, | ||
| { | ||
| type Event = Arc<HotShotEvent<TYPES>>; | ||
|
|
||
| type Output = HotShotTaskCompleted; | ||
|
|
||
| async fn handle_event(event: Self::Event, task: &mut Task<Self>) -> Option<Self::Output> { | ||
| let sender = task.clone_sender(); | ||
| task.state_mut().handle_event(event, &sender).await | ||
| } | ||
|
|
||
| fn should_shutdown(event: &Self::Event) -> bool { | ||
| matches!(event.as_ref(), HotShotEvent::Shutdown) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think implementing TaskState here (and calling the function handle_event below) was misleading, since we were never spawning the vote task as a task.
This may be a CompletableTaskState in the future, but that is outside the scope of this PR.
ss-es
commented
May 18, 2024
Comment on lines
-59
to
-78
| /// Task state for a test. Similar to `TaskState` but it handles | ||
| /// messages as well as events. Messages are events that are | ||
| /// external to this task. (i.e. a test message would be an event from non test task) | ||
| /// This is used as state for `TestTask` and messages can come from many | ||
| /// different input streams. | ||
| pub trait TestTaskState: Send { | ||
| /// Message type handled by the task | ||
| type Message: Clone + Send + Sync + 'static; | ||
| /// Result returned by the test task on completion | ||
| type Output: Send; | ||
| /// The state type | ||
| type State: TaskState; | ||
| /// Handle and incoming message and return `Some` if the task is finished | ||
| fn handle_message( | ||
| message: Self::Message, | ||
| id: usize, | ||
| task: &mut TestTask<Self::State, Self>, | ||
| ) -> impl Future<Output = Option<Self::Output>> + Send | ||
| where | ||
| Self: Sized; |
There was a problem hiding this comment.
TestTaskState and TestTask are moved to testing/src/test_task.rs.
The trait and struct themselves have been somewhat reworked, but should fit into the test runner in the same way.
jparr721
previously approved these changes
May 24, 2024
jparr721
previously approved these changes
May 27, 2024
jparr721
previously approved these changes
May 28, 2024
jparr721
approved these changes
May 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #3208
The general goal of this PR is to make it possible to halt tasks, retrieve their state and restart them.
I think this would be nice for a number of reasons, but in particular it would allow us to very easily upgrade
NodeTypeat runtime.This PR:
TaskStatetrait: implementations must now only specify an associatedEventtype, thehandle_eventfunction and acancel_subtasksfunction to cleanup any spawned subtasks.TaskStateimplementations removed, and will be subsumed under a newNetworkTaskStatein a future PR.TaskStateobject via aJoinHandle. TheseJoinHandles are collected in aConsensusTaskRegistry.JoinHandle<()>are collected in aNetworkTaskRegistry, and will be dealt with in a future PR.TaskState, because they fundamentally do not listen on a single receiver in the same way that consensus tasks do.TestTaskStaterequires acheck()function that returns the pass/fail result of the test.Minor:
RwLockandArcindirection in task registries is removed, and these now simply contain aVecofJoinHandles. As a consequence,SystemContextHandleis not clonable; this is a mildly dangerous operation anyway, since a HotShot handle is a unique object and cloning it does not create a new one, and makes ownership harder to reason about, especially around mutable operations likeshut_down. An application that wishes to hold multiple handles should put the handle it receives into aArc<RwLock<...>>and synchronize calls to the handle itself.shut_downis no longer aBoxSynFuture, because shutting down the consensus registry is notSync(it must await on theJoinHandles it holds)SystemContextHandle; they only needed 1-2 fields, so we simply pass these directly to avoid additional the additionalConsensusApiconstraint.This PR does not:
There should not be any external, observable changes in runtime behavior.
Key places to review:
This PR is quite large because of the scope of the changes, but the vast majority of it is rewiring. I think the most significant intentional changes are in:
task/src/task.rs: the main trait changes. I believe it is easier to simply read the final file, rather than the diff.testing/src/test_task.rs: the test task changes. Note thatTestTaskand theTestTaskStatetrait were previously located intask/src/task.rs, but these were essentially rewritten from scratch.But it's definitely worth looking at other files if you can. If there's anything you suspect might be an unintentional or incorrect logic change, please flag it.