Skip to content

fix: add a 5 second timeout for tokio_runtime shutdown#8771

Merged
mattsse merged 5 commits intomainfrom
joshie/timeout-rt-shut
Jun 12, 2024
Merged

fix: add a 5 second timeout for tokio_runtime shutdown#8771
mattsse merged 5 commits intomainfrom
joshie/timeout-rt-shut

Conversation

@joshieDo
Copy link
Collaborator

@joshieDo joshieDo commented Jun 12, 2024

Waits 5 seconds for tokio_runtime to shutdown. Making a best effort to allow components to have a chance to drop cleanly should be preferred. Examples: allows for StorageLock file to be cleared (not a must) or errors to be properly printed (eg. #8772).

Acknowledging that for long blocking tasks, this will timeout anyway. (eg. sender recovery stage)

Also, reduces task_manager shutdown timeout to 5 seconds as to not increase the worst case waiting time.

@joshieDo joshieDo added C-bug An unexpected or incorrect behavior A-cli Related to the reth CLI labels Jun 12, 2024
@joshieDo joshieDo requested review from mattsse and onbjerg as code owners June 12, 2024 13:27
@joshieDo joshieDo changed the title fix: wait 5 seconds for tokio_runtime to end on a graceful shutdown attempt fix: add a 5 second timeout for tokio_runtime shutdown Jun 12, 2024
@joshieDo joshieDo requested a review from mattsse June 12, 2024 14:26
Copy link
Collaborator

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

can you include the motivation in the pr?

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

tested this a few times, seems fine

// manager which fires the shutdown signal to all tasks spawned via the task
// executor and awaiting on tasks spawned with graceful shutdown
task_manager.graceful_shutdown_with_timeout(std::time::Duration::from_secs(10));
task_manager.graceful_shutdown_with_timeout(Duration::from_secs(5));
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems reasonable, none of our graceful tasks should even take 1s to shutdown

@mattsse mattsse enabled auto-merge June 12, 2024 18:35
@mattsse mattsse added this pull request to the merge queue Jun 12, 2024
Merged via the queue into main with commit 7b58643 Jun 12, 2024
@mattsse mattsse deleted the joshie/timeout-rt-shut branch June 12, 2024 19:02
emhane pushed a commit that referenced this pull request Jun 13, 2024
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Related to the reth CLI C-bug An unexpected or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments