Skip to content

Replace restic.Progress with new progress.Counter (fixes two race conditions)#3058

Merged
MichaelEischer merged 2 commits intorestic:masterfrom
greatroar:counter
Nov 9, 2020
Merged

Replace restic.Progress with new progress.Counter (fixes two race conditions)#3058
MichaelEischer merged 2 commits intorestic:masterfrom
greatroar:counter

Conversation

@greatroar
Copy link
Copy Markdown
Contributor

@greatroar greatroar commented Nov 4, 2020

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

This PR replaces restic.Progress with a new type, ui/progress.Counter. It fixes several issues:

  • Progress.Done races with Progress.reporter for access to Progress.fnM. As a result, a progress report can be printed after the Progress has been canceled.
  • Progress.Done does not wait for termination, so it races with other routines for stdout, or the final message may not be printed before the process exits.
  • Initialization is spread out over the constructor and various init functions, that are called even for processes that do not use a Progress.
  • Progress is defined in internal/restic, overloading the meaning of that package.
  • Callbacks may be called either from a separate goroutine or from Progress.Report's caller's goroutine. This was probably meant as an optimization, but it means that the caller may have to wait on stdout.
  • Progress is designed to handle restic.Stat objects, which carry counts of various object types. Its callers (ab)use only its blobs count for various purposes, most of which are not counting blobs.
  • The code has grown organically over time, making it complicated and leaving dead code in various places.

Instead of fixing these issues one by one, I've decided to write a replacement from scratch. It's >100 lines shorter, leaving ample space for a unit test that covers 92% of its statements.

Was the change discussed in an issue or in the forum before?

No.

Checklist

  • I have read the Contribution Guidelines
  • I have enabled maintainer edits for this PR
  • I have added tests for all changes in this PR
  • I have added documentation for the changes (in the manual)
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • 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

@greatroar greatroar force-pushed the counter branch 6 times, most recently from 1a8ac56 to 5d6426d Compare November 5, 2020 10:49
Copy link
Copy Markdown
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

Really nice cleanup! I've just a small remark:
I'd like to keep the progress bar creation and the calls to Done() together on the same level to consistently let the progress bar starter stop it. Besides that the code changes are fine.

@greatroar
Copy link
Copy Markdown
Contributor Author

Moved Done() to the call sites of newProgressMax. Index.Load did not have a call site outside of tests. Also removed the Counter argument from checker.ReadData, since that was always called with a nil Counter.

@greatroar
Copy link
Copy Markdown
Contributor Author

Btw., is it ok if I change the default FPS from 60 to, say, 30, or even 20? It's not like we're trying to create a fluent animation here.

@greatroar greatroar force-pushed the counter branch 4 times, most recently from cfab254 to 6f78747 Compare November 9, 2020 10:32
This fixes two race conditions while cleaning up the code.
Copy link
Copy Markdown
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for cleaning this up. I'd say we just stick with the 60 fps for now. After all it shouldn't cost too much performance.

@MichaelEischer MichaelEischer merged commit 46d31ab into restic:master Nov 9, 2020
@greatroar greatroar deleted the counter branch November 10, 2020 08:13
MichaelEischer added a commit to MichaelEischer/restic that referenced this pull request Dec 28, 2020
This reverts to the old behavior of not printing progress updates on
non-interactive terminals. It was accidentally changed in restic#3058.
MichaelEischer added a commit to MichaelEischer/restic that referenced this pull request Dec 29, 2020
This reverts to the old behavior of not printing progress updates on
non-interactive terminals. It was accidentally changed in restic#3058.
mfrischknecht pushed a commit to mfrischknecht/restic that referenced this pull request Jun 14, 2022
This reverts to the old behavior of not printing progress updates on
non-interactive terminals. It was accidentally changed in restic#3058.
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.

2 participants