Replace task `run()` return result with custom enum #2429

MitchTurner · 2024-11-12T08:15:47Z

Linked Issues/PRs

closes: #2441

Description

Before any RunnableTask would return a Result<bool> where, implicitly, the bool was telling the service runner if it should re-run the task.

This is confusing from the customer perspective since you need to understand the behavior of ServiceRunner in order to choose the right response: i.e. Ok(true) meant you want to rerun. Instead this PR adds the Continue and Stop variants.

It was also confusing with Error since, unintuitively, an Error would result in the task being run again. This PR adds the ErrorContinue variant, so the implementer know explicitly what the behavior is.

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

MitchTurner · 2024-11-12T11:13:09Z

crates/services/consensus_module/poa/src/sync.rs

@@ -137,8 +138,7 @@ impl RunnableService for SyncTask {

 #[async_trait::async_trait]
 impl RunnableTask for SyncTask {
-    #[tracing::instrument(level = "debug", skip_all, err, ret)]


Had to remove this from a couple locations since the instrument macro assumes the function returns Result

MitchTurner · 2024-11-12T11:17:18Z

crates/fuel-core/src/graphql_api/api_service.rs

@@ -196,11 +197,15 @@ impl RunnableService for GraphqlService {

 #[async_trait::async_trait]
 impl RunnableTask for Task {
-    async fn run(&mut self, _: &mut StateWatcher) -> anyhow::Result<bool> {
-        self.server.as_mut().await?;


This change introduces a non-trivial amount of noise with the removal of the ? operators.

AFAICT, there isn't a way to implement Try for arbitrary types in Rust which is a real bummer.

crates/services/p2p/src/service.rs

xgreenx · 2024-11-13T19:17:05Z

crates/fuel-core/src/service/adapters/consensus_parameters_provider.rs

                        }
                    }
                }
                should_continue = true;
            }
        }

-        Ok(should_continue)
+        TaskRunResult::should_continue(should_continue)


I though the idea was to remove should_continue variable and replace with the usage of the enum=D

My goal was to change the interface.

I'm happy to do some more refactoring too.

xgreenx · 2024-11-13T19:25:06Z

crates/services/gas_price_service/src/v1/da_source_service/service.rs

-                let da_block_costs = self.source.request_da_block_cost().await?;
-                self.shared_state.0.send(da_block_costs)?;
-                continue_running = true;
+                match self.source.request_da_block_cost().await.and_then(|da_block_costs| self.shared_state.0.send(da_block_costs).map_err(|err| anyhow::anyhow!(err))) {


It is very hard to read, maybe we could it inside of the function and process the result of the function?=)

xgreenx · 2024-11-13T19:26:59Z

crates/services/gas_price_service/src/v0/service.rs

+                let block = match l2_block_res {
+                    Ok(block) => block,
+                    Err(err) => {
+                        return anyhow!(err).into()
+                    }
+                };


I see this pattern in many places. Maybe it would be simpler to defined a helper trait with into_continue_task_result which either returns TaskRunResult::Continue or TaskRunResult::ErrorContinue

crates/services/src/service.rs

acerone85 · 2024-11-15T12:06:46Z

crates/services/src/service.rs

+                tracing::debug!("stopping");
+                break;
+            }
+            TaskRunResult::ErrorContinue(e) => {
                let e: &dyn std::error::Error = &*e;
                tracing::error!(e);


would it make sense to add a debug! statement to explicitly tell that the loop will continue (similar to the Continue case)?

I'm not sure I understand. Are you talking in the ErrorContinue case? I feel like we should definitely use error! there, especially since we don't exit on errors.

xgreenx · 2024-11-16T21:55:06Z

crates/services/gas_price_service/src/v1/service.rs

+                let res = self.process_da_block_costs_res(da_block_costs).await;
+                TaskRunResult::Continue


If we have error here, you will not propagate it to the caller of the run function and we will miss log for this error.

Thank you for catching this.

xgreenx · 2024-11-16T21:56:14Z

crates/services/p2p/src/service.rs

@@ -916,7 +915,12 @@ where
                    }
                    Some(TaskRequest::RespondWithGossipsubMessageReport((message, acceptance))) => {
                        // report_message(&mut self.p2p_service, message, acceptance);
-                        self.p2p_service.report_message(message, acceptance)?;
+                        match self.p2p_service.report_message(message, acceptance) {


[nit] if let Err(err) = res fits better here.

xgreenx · 2024-11-16T21:57:45Z

crates/services/src/service.rs

+
+impl TaskRunResult {
+    /// Creates a `TaskRunResult` from a `Result` where `Ok` means `Continue` and any error is reported
+    pub fn continue_if_ok<T, E: Into<anyhow::Error>>(res: Result<T, E>) -> TaskRunResult {


continue_if_ok is a bad name. I thought it returns Continue only If it is Ok.

What do you think about always_continue?

I thought it returns Continue only If it is Ok.

Yeah. It returns continue_ only if_ it is ok. I doesn't always_ return continue... I'm confused why you want me to change it.

It returns Continue or ErrorContinue, which continues the task even if an error occurred.

From the function with name continue_if_ok I would expect return Continue on Ok and return Stop on Err

I see now. That is confusing. I feel like "always continue" is confusing for the opposite reason though. "continue or error"? Haha

Maybe just continue then?=) For me, the main point here is that we should still continue the task. What happens with error is not important for me as a reader of the code.

Yeah. I think you're right. I think that's what the caller is interested in. I'll go with one of those.

I feel like TaskRunResult is the wrong name then. It's almost a state machine but not quite. But TaskState or TaskStateTransition might be a better name...

I went with always_continue and TaskNextAction (I asked ChatGPT for a good name and that was the one I liked the most)

rafal-ch

LGTM 👍
with some nits/suggestions

Anyway, good piece of QoL code :)

crates/services/src/service.rs

crates/services/consensus_module/poa/src/service.rs

crates/services/p2p/src/service.rs

acerone85 · 2024-11-19T10:01:11Z

crates/fuel-core/src/graphql_api/worker_service.rs

@@ -568,16 +568,19 @@ where
                    // de-synchronization between on-chain and off-chain databases.
                    if let Err(e) = result {
                        tracing::error!("Error processing block: {:?}", e);
-                        should_continue = self.continue_on_error;
+                        if self.continue_on_error {
+                            TaskNextAction::Continue


why not ErrorContinue here?

Yeah. Good idea. I was going to say that I want to maintain the old behavior, but that's essentially the same and cleaner.

MitchTurner added 4 commits November 12, 2024 15:14

WIP: Replace result with custom enum

f0ac16d

Finish converting

48d752b

Update CHANGELOG

0e8720a

Fix spelling--unrelated to branch purpose

d1c5a2a

MitchTurner commented Nov 12, 2024

View reviewed changes

crates/services/p2p/src/service.rs Outdated Show resolved Hide resolved

MitchTurner added 3 commits November 12, 2024 18:23

Remove redundant returns

894d8de

Update crates/services/p2p/src/service.rs

81e1a90

Revert file back to master

b091adf

MitchTurner marked this pull request as ready for review November 12, 2024 11:26

MitchTurner requested review from AurelienFT, xgreenx and Dentosal as code owners November 12, 2024 11:26

xgreenx reviewed Nov 13, 2024

View reviewed changes

MitchTurner self-assigned this Nov 14, 2024

MitchTurner added 5 commits November 15, 2024 14:26

Merge branch 'master' into chore/introduce-task-result-enum

3b3b9cd

Fix new compilation errors from master merge

4648b49

Add helper to v0 code

78c2940

Cleanup noisy code

81c9d22

Remove should_continue method

8495bc9

acerone85 reviewed Nov 15, 2024

View reviewed changes

crates/services/src/service.rs Show resolved Hide resolved

acerone85 reviewed Nov 15, 2024

View reviewed changes

xgreenx reviewed Nov 16, 2024

View reviewed changes

MitchTurner added 5 commits November 17, 2024 14:32

Merge branch 'master' into chore/introduce-task-result-enum

268bd1c

Cover missing error case, reformat expression

3538282

Rename function and type

f8e9649

Add missed rename

63b45b3

Merge branch 'master' into chore/introduce-task-result-enum

747030c

xgreenx previously approved these changes Nov 18, 2024

View reviewed changes

Merge branch 'master' into chore/introduce-task-result-enum

cea56f7

MitchTurner requested a review from a team November 18, 2024 12:41

rafal-ch previously approved these changes Nov 19, 2024

View reviewed changes

crates/services/src/service.rs Show resolved Hide resolved

crates/services/consensus_module/poa/src/service.rs Outdated Show resolved Hide resolved

crates/services/p2p/src/service.rs Show resolved Hide resolved

acerone85 reviewed Nov 19, 2024

View reviewed changes

acerone85 previously approved these changes Nov 19, 2024

View reviewed changes

Merge branch 'master' into chore/introduce-task-result-enum

9481e5c

MitchTurner dismissed stale reviews from acerone85, rafal-ch, and xgreenx via 9481e5c November 19, 2024 10:06

Make minor touch-ups

948dfb8

MitchTurner requested review from xgreenx, acerone85 and rafal-ch November 19, 2024 10:20

rafal-ch approved these changes Nov 19, 2024

View reviewed changes

acerone85 approved these changes Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace task `run()` return result with custom enum #2429

Replace task `run()` return result with custom enum #2429

MitchTurner commented Nov 12, 2024 •

edited

Loading

MitchTurner Nov 12, 2024 •

edited

Loading

MitchTurner Nov 12, 2024

xgreenx Nov 13, 2024

MitchTurner Nov 15, 2024

xgreenx Nov 13, 2024

xgreenx Nov 13, 2024

acerone85 Nov 15, 2024

MitchTurner Nov 17, 2024

xgreenx Nov 16, 2024

MitchTurner Nov 17, 2024

xgreenx Nov 16, 2024

xgreenx Nov 16, 2024

MitchTurner Nov 17, 2024

xgreenx Nov 17, 2024

MitchTurner Nov 17, 2024

xgreenx Nov 17, 2024

MitchTurner Nov 17, 2024

MitchTurner Nov 17, 2024

MitchTurner Nov 17, 2024

rafal-ch left a comment

acerone85 Nov 19, 2024

MitchTurner Nov 19, 2024

		let res = self.process_da_block_costs_res(da_block_costs).await;
		TaskRunResult::Continue

Replace task run() return result with custom enum #2429

Are you sure you want to change the base?

Replace task run() return result with custom enum #2429

Conversation

MitchTurner commented Nov 12, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

MitchTurner Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafal-ch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Replace task `run()` return result with custom enum #2429

Replace task `run()` return result with custom enum #2429

MitchTurner commented Nov 12, 2024 •

edited

Loading

MitchTurner Nov 12, 2024 •

edited

Loading