Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cancel sync properly on shutdown #4380

Merged
merged 5 commits into from
Aug 24, 2022
Merged

Cancel sync properly on shutdown #4380

merged 5 commits into from
Aug 24, 2022

Conversation

flcl42
Copy link
Contributor

@flcl42 flcl42 commented Aug 8, 2022 •
edited
Loading

Fixes | Closes | Resolves #4049

Fast headers sync requires heavy database writes on request preparation which was not interruptible before this fix

Changes:

  • Interrupt fast headers sync properly

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

@LukaszRozmej
Copy link
Member

LukaszRozmej commented Aug 8, 2022 •
edited
Loading

Not sure about this, how much time docker gives the application to gracefully terminate before it terminates it hard? I know its configurable but there is some default.

I think default is 10s: https://docs.docker.com/engine/reference/commandline/stop/

@LukaszRozmej
Copy link
Member

What happens if not all data is flushed? How much time is needed on average to flush data? Which feed is the most problematic?

@LukaszRozmej LukaszRozmej marked this pull request as draft August 10, 2022 14:50
@flcl42 flcl42 marked this pull request as ready for review August 12, 2022 15:23
@flcl42 flcl42 force-pushed the bugfix/extend-timeout-to-save-all-data branch from 1d39214 to 6b7180f Compare August 12, 2022 18:58
@asdacap
Copy link
Contributor

asdacap commented Aug 14, 2022

BlockDownloader and MergeBlockDownloader is long running, meaning it's dispatch have a long running loop, you may want to check them out, pass a cancellation token to the dispatch or something.

@asdacap
Copy link
Contributor

asdacap commented Aug 14, 2022

Nevermind, seems to have already been accounted for.

@flcl42 flcl42 force-pushed the bugfix/extend-timeout-to-save-all-data branch from 6b7180f to 0077cbb Compare August 15, 2022 11:51
@flcl42 flcl42 changed the base branch from master to develop August 16, 2022 15:00
@flcl42 flcl42 force-pushed the bugfix/extend-timeout-to-save-all-data branch from 0077cbb to a84437b Compare August 16, 2022 15:02
@flcl42 flcl42 force-pushed the bugfix/extend-timeout-to-save-all-data branch from a84437b to 6ee5d18 Compare August 18, 2022 10:26
@flcl42 flcl42 changed the base branch from develop to master August 18, 2022 15:43
@flcl42 flcl42 changed the title Extend time to flush the data Cancel sync properly on shutdown Aug 18, 2022
LukaszRozmej
LukaszRozmej approved these changes Aug 24, 2022
View reviewed changes
src/Nethermind/Nethermind.Synchronization/ParallelSync/SyncDispatcher.cs Outdated
@@ -113,6 +106,11 @@ public async Task Start(CancellationToken cancellationToken)
if (Logger.IsWarn) Logger.Warn($"Failure when executing request {t.Exception}");
}

if (cancellationToken.IsCancellationRequested)
{
if (Logger.IsInfo) Logger.Info("Ignoring sync response as shutdown is requested.");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be Debug/Trace?

LukaszRozmej
LukaszRozmej reviewed Aug 24, 2022
View reviewed changes
src/Nethermind/Nethermind.Synchronization/ParallelSync/SyncDispatcher.cs Outdated
Comment on lines 109 to 113
if (cancellationToken.IsCancellationRequested)
{
if (Logger.IsDebug) Logger.Debug("Ignoring sync response as shutdown is requested.");
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

I would put this inside the below try to still have call to Free(allocation). It probably won't matter for shutdown but potentially if we have other places where we would like to cancel the sync - yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixing

@flcl42 flcl42 merged commit 6c32698 into master Aug 24, 2022
@flcl42 flcl42 deleted the bugfix/extend-timeout-to-save-all-data branch August 24, 2022 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dceleda dceleda Awaiting requested review from dceleda

@LukaszRozmej LukaszRozmej Awaiting requested review from LukaszRozmej

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nethermind error when shutting down during snap sync
3 participants
@flcl42 @LukaszRozmej @asdacap