Skip to content

Replace cleanup handlers with context based command cancelation#4753

Merged
MichaelEischer merged 10 commits intorestic:masterfrom
MichaelEischer:remove-cleanup-handlers
Apr 24, 2024
Merged

Replace cleanup handlers with context based command cancelation#4753
MichaelEischer merged 10 commits intorestic:masterfrom
MichaelEischer:remove-cleanup-handlers

Conversation

@MichaelEischer
Copy link
Copy Markdown
Member

What does this PR change? What problem does it solve?

Restic has historically relied on a homegrown mechanism using cleanup handlers to handle command interrupts like SIGINT. This PR completely removes that mechanism and only uses Contexts to cancel commands. This removes another source of global state from cmd/restic/ and might be useful in the future as it allows for a controlled shutdown of commands compared to the current approach that is more akin to a "controlled crash".

Due to the cleanup handlers approach there was no need to ensure that every command exits as fast as possible once a context has been canceled. This PR therefore adds lots of checks for canceled contexts. Previously, if the cleanup handlers took too long, then a command like prune could in some cases erroneously report a corrupted repository.

From a user perspective there shouldn't be any large changes, although the text output of an interrupted command will change slightly.

There are two ugly parts in the current implementation: reading a password and serving a fuse mount appear to offer no nice way to honor a canceled context. To still somewhat integrate with context cancellation, the blocking command runs in a separate goroutine that is leaked if the command gets canceled. This is good enough for uses in restic, as canceling the context will result in program termination anyways.

Was the change previously discussed in an issue or on the forum?

Mentioned as part of the restic 0.17.0 roadmap. Prerequisite for #4406 .

Checklist

  • I have read the contribution guidelines.
  • I have enabled maintainer edits.
  • I have added tests for all code changes.
  • [ ] I have added documentation for relevant changes (in the manual).
  • [ ] There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
  • I have run gofmt on the code in all commits.
  • All commit messages are formatted in the same style as the other commits in the repo.
  • I'm done! This pull request is ready for review.

@MichaelEischer MichaelEischer force-pushed the remove-cleanup-handlers branch 5 times, most recently from 8169a0f to 0ac5e1a Compare April 10, 2024 19:26
@MichaelEischer MichaelEischer force-pushed the remove-cleanup-handlers branch from 0ac5e1a to 484dbb1 Compare April 22, 2024 20:40
@MichaelEischer
Copy link
Copy Markdown
Member Author

LGTM

@MichaelEischer MichaelEischer merged commit a7b5e09 into restic:master Apr 24, 2024
@MichaelEischer MichaelEischer deleted the remove-cleanup-handlers branch April 24, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant