-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Replace task run()
return result with custom enum
#2429
base: master
Are you sure you want to change the base?
Changes from 7 commits
f0ac16d
48d752b
0e8720a
d1c5a2a
894d8de
81e1a90
b091adf
3b3b9cd
4648b49
78c2940
81c9d22
8495bc9
268bd1c
3538282
f8e9649
63b45b3
747030c
cea56f7
9481e5c
948dfb8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ use fuel_core_services::{ | |
ServiceRunner, | ||
SharedMutex, | ||
StateWatcher, | ||
TaskRunResult, | ||
}; | ||
use fuel_core_storage::{ | ||
not_found, | ||
|
@@ -111,7 +112,7 @@ impl SharedState { | |
|
||
#[async_trait::async_trait] | ||
impl RunnableTask for Task { | ||
async fn run(&mut self, watcher: &mut StateWatcher) -> anyhow::Result<bool> { | ||
async fn run(&mut self, watcher: &mut StateWatcher) -> TaskRunResult { | ||
let should_continue; | ||
tokio::select! { | ||
biased; | ||
|
@@ -135,16 +136,15 @@ impl RunnableTask for Task { | |
} | ||
Err(err) => { | ||
tracing::error!("Failed to cache consensus parameters: {:?}", err); | ||
should_continue = false; | ||
return Ok(should_continue) | ||
return TaskRunResult::Stop | ||
} | ||
} | ||
} | ||
should_continue = true; | ||
} | ||
} | ||
|
||
Ok(should_continue) | ||
TaskRunResult::should_continue(should_continue) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I though the idea was to remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My goal was to change the interface. I'm happy to do some more refactoring too. |
||
} | ||
|
||
async fn shutdown(self) -> anyhow::Result<()> { | ||
|
@@ -358,7 +358,7 @@ mod tests { | |
// When | ||
let result_with_new_version = result_with_new_version(new_version); | ||
let _ = block_sender.send(result_with_new_version); | ||
task.run(&mut StateWatcher::started()).await.unwrap(); | ||
task.run(&mut StateWatcher::started()).await; | ||
|
||
// Then | ||
assert_eq!( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ use fuel_core_services::{ | |
RunnableService, | ||
RunnableTask, | ||
StateWatcher, | ||
TaskRunResult, | ||
}; | ||
use fuel_core_types::{ | ||
blockchain::header::BlockHeader, | ||
|
@@ -137,8 +138,7 @@ impl RunnableService for SyncTask { | |
|
||
#[async_trait::async_trait] | ||
impl RunnableTask for SyncTask { | ||
#[tracing::instrument(level = "debug", skip_all, err, ret)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Had to remove this from a couple locations since the |
||
async fn run(&mut self, watcher: &mut StateWatcher) -> anyhow::Result<bool> { | ||
async fn run(&mut self, watcher: &mut StateWatcher) -> TaskRunResult { | ||
let mut should_continue = true; | ||
|
||
let tick: BoxFuture<tokio::time::Instant> = if let Some(timer) = &mut self.timer { | ||
|
@@ -218,7 +218,7 @@ impl RunnableTask for SyncTask { | |
} | ||
} | ||
|
||
Ok(should_continue) | ||
TaskRunResult::should_continue(should_continue) | ||
} | ||
|
||
async fn shutdown(self) -> anyhow::Result<()> { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ use async_trait::async_trait; | |
use fuel_core_services::{ | ||
RunnableTask, | ||
StateWatcher, | ||
TaskRunResult, | ||
}; | ||
use fuel_gas_price_algorithm::v0::{ | ||
AlgorithmUpdaterV0, | ||
|
@@ -121,24 +122,32 @@ where | |
L2: L2BlockSource, | ||
Metadata: MetadataStorage, | ||
{ | ||
async fn run(&mut self, watcher: &mut StateWatcher) -> anyhow::Result<bool> { | ||
let should_continue; | ||
async fn run(&mut self, watcher: &mut StateWatcher) -> TaskRunResult { | ||
tokio::select! { | ||
biased; | ||
_ = watcher.while_started() => { | ||
tracing::debug!("Stopping gas price service"); | ||
should_continue = false; | ||
TaskRunResult::Stop | ||
} | ||
l2_block_res = self.l2_block_source.get_l2_block() => { | ||
tracing::info!("Received L2 block result: {:?}", l2_block_res); | ||
let block = l2_block_res?; | ||
let block = match l2_block_res { | ||
Ok(block) => block, | ||
Err(err) => { | ||
return anyhow!(err).into() | ||
} | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see this pattern in many places. Maybe it would be simpler to defined a helper trait with |
||
|
||
tracing::debug!("Updating gas price algorithm"); | ||
self.apply_block_info_to_gas_algorithm(block).await?; | ||
should_continue = true; | ||
match self.apply_block_info_to_gas_algorithm(block).await { | ||
Ok(_) => {}, | ||
Err(err) => { | ||
return err.into() | ||
} | ||
} | ||
TaskRunResult::Continue | ||
} | ||
} | ||
Ok(should_continue) | ||
} | ||
|
||
async fn shutdown(mut self) -> anyhow::Result<()> { | ||
|
@@ -253,7 +262,7 @@ mod tests { | |
let initial_price = read_algo.next_gas_price(); | ||
|
||
// when | ||
service.run(&mut watcher).await.unwrap(); | ||
service.run(&mut watcher).await; | ||
l2_block_sender.send(l2_block).await.unwrap(); | ||
service.shutdown().await.unwrap(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ use fuel_core_services::{ | |
RunnableTask, | ||
ServiceRunner, | ||
StateWatcher, | ||
TaskRunResult, | ||
}; | ||
use std::time::Duration; | ||
use tokio::{ | ||
|
@@ -100,21 +101,23 @@ where | |
{ | ||
/// This function polls the source according to a polling interval | ||
/// described by the DaBlockCostsService | ||
async fn run(&mut self, state_watcher: &mut StateWatcher) -> Result<bool> { | ||
let continue_running; | ||
|
||
async fn run(&mut self, state_watcher: &mut StateWatcher) -> TaskRunResult { | ||
tokio::select! { | ||
biased; | ||
_ = state_watcher.while_started() => { | ||
continue_running = false; | ||
TaskRunResult::Stop | ||
} | ||
_ = self.poll_interval.tick() => { | ||
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))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is very hard to read, maybe we could it inside of the function and process the result of the function?=) |
||
Ok(da_block_costs) => { | ||
TaskRunResult::Continue | ||
} | ||
Err(err) => { | ||
TaskRunResult::ErrorContinue(err) | ||
} | ||
} | ||
} | ||
} | ||
Ok(continue_running) | ||
} | ||
|
||
/// There are no shutdown hooks required by the sources *yet* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.