search

GoogleCloudPlatform / gcr-cleaner

Delete untagged image refs in Google Container Registry or Artifact Registry
Apache License 2.0
805 stars 112 forks source link

Alternate fix to duplicate SHAs #72

Closed nWhitehill closed 2 years ago

nWhitehill commented 2 years ago

Description

Fixes the duplicate and incorrect SHA output by placing the manifest to be deleted in an anonymous function's parameter and immediately invoking. IMO this makes the call stack more clear and safer. Any future refactors can access any of the parameters on the manifest variable without worrying about thread safety. That said, I completely understand this is a nitpick. This just came about from me debugging why v0.7.0 was printing the same SHA and sometimes even the wrong SHA occasionally. However, I still wanted to propose the revision to the community.

Background

Commit 834f5cbd9 fixed an issue where duplicate and potentially wrong SHAs were output. The crux of the bug was that the pool functions tried to access the m variable that was declared in the for loop. Since this pointer changed what manifest it was pointing at, it wasn't thread safe. The pool functions would just output the digest of the manifest they were on, This usually manifested as duplicates but sometimes as the wrong SHA.

The fix for this issue was to store the ref variable directly on the call stack. Since the pool functions no longer were using a pointer of a pointer (m.Digest) that changes during iteration, the calls are safe

google-cla[bot] commented 2 years ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.