Skip to content

Conversation

@sapinb
Copy link
Contributor

@sapinb sapinb commented Aug 16, 2024

Description

Following similar pattern to how Reth's task manager is implemented

  • Add TaskManager and TaskExecutor
  • Pass ShutdownGuard to all tasks, tasks can check on this to check for shutdown signal
  • Signal shutdown on main task panics and external interrupts
  • Timeout (currently 5s) to wait for tasks to end after shutdown triggered

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactor

Checklist

  • I have performed a self-review of my code.
  • I have commented my code where necessary.
  • I have updated the documentation if needed.
  • My changes do not introduce new warnings.
  • I have added tests that prove my changes are effective or that my feature works.
  • New and existing tests pass with my changes.

Related Issues

Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

Looks good, should we re-export this in common and/or maybe move it under util?

@sapinb sapinb force-pushed the EXP-27-watch-core-tasks branch from 3676093 to c9d0ec7 Compare August 20, 2024 17:00
@sapinb sapinb requested a review from delbonis August 20, 2024 17:20
@sapinb sapinb marked this pull request as ready for review August 20, 2024 17:21
Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

Main issue is that this treats async tasks like sync ones, expecting them to poll a handle when we should drop them directly with Abortable. Secondarily, sync tasks that wait on a channel are a bit tricky to deal with here since we'd somehow want to interrupt the channel recv so that we can monitor the guard in blocking way. A lazy way to do this is with a timeout but there might be a better one that's not too invasive.

@sapinb sapinb requested a review from delbonis August 20, 2024 20:40
delbonis
delbonis previously approved these changes Aug 20, 2024
Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

Minor question, looks good otherwise.

@codecov
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 66.47834% with 178 lines in your changes missing coverage. Please review.

Project coverage is 55.07%. Comparing base (1485d2e) to head (f617586).
Report is 21 commits behind head on master.

Files Patch % Lines
sequencer/src/main.rs 0.00% 55 Missing ⚠️
crates/tasks/src/manager.rs 91.42% 30 Missing ⚠️
crates/btcio/src/writer/writer_handler.rs 0.00% 27 Missing ⚠️
crates/consensus-logic/src/duty/worker.rs 0.00% 18 Missing ⚠️
sequencer/src/l1_reader.rs 0.00% 16 Missing ⚠️
crates/consensus-logic/src/fork_choice_manager.rs 0.00% 14 Missing ⚠️
crates/consensus-logic/src/sync_manager.rs 0.00% 6 Missing ⚠️
crates/consensus-logic/src/worker.rs 0.00% 5 Missing ⚠️
crates/consensus-logic/src/l1_handler.rs 0.00% 3 Missing ⚠️
crates/tasks/src/shutdown.rs 91.66% 3 Missing ⚠️
... and 1 more
@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
+ Coverage   54.15%   55.07%   +0.91%     
==========================================
  Files         111      113       +2     
  Lines       11964    12404     +440     
==========================================
+ Hits         6479     6831     +352     
- Misses       5485     5573      +88     
Files Coverage Δ
crates/btcio/src/reader/query.rs 0.00% <ø> (ø)
crates/btcio/src/writer/watcher.rs 0.00% <ø> (ø)
crates/db/src/traits.rs 100.00% <ø> (ø)
sequencer/src/rpc_server.rs 0.00% <0.00%> (ø)
crates/consensus-logic/src/l1_handler.rs 0.00% <0.00%> (ø)
crates/tasks/src/shutdown.rs 91.66% <91.66%> (ø)
crates/consensus-logic/src/worker.rs 0.00% <0.00%> (ø)
crates/consensus-logic/src/sync_manager.rs 0.00% <0.00%> (ø)
crates/consensus-logic/src/fork_choice_manager.rs 0.00% <0.00%> (ø)
sequencer/src/l1_reader.rs 0.00% <0.00%> (ø)
... and 4 more

... and 1 file with indirect coverage changes

@sapinb sapinb requested a review from storopoli August 22, 2024 06:36
Rajil1213
Rajil1213 previously approved these changes Aug 22, 2024
storopoli
storopoli previously approved these changes Aug 22, 2024
Copy link
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

It can be merged as it is.

I have some minor nits and a big question on UnwindSafe bounds that I do know the answer.

@sapinb sapinb dismissed stale reviews from storopoli and Rajil1213 via 03c6442 August 22, 2024 12:09
Copy link
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

ACK f617586

@Rajil1213 Rajil1213 merged commit 16d011f into master Aug 22, 2024
@storopoli storopoli deleted the EXP-27-watch-core-tasks branch November 28, 2024 10:31
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.

5 participants