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.

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

Alternate fix to duplicate SHAs #72

Merged

Conversation

nWhitehill
Copy link
Contributor

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
Copy link

google-cla bot commented Mar 22, 2022

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.

@nWhitehill nWhitehill mentioned this pull request Mar 22, 2022
Saves a copy of the manifest that is to be deleted for each thread in the worker pool. Prevents issue where the range struct is accessed with values for a manifest that are different from when the delete async function was added to the pool.
@nWhitehill nWhitehill force-pushed the feature/duplicateOutputGenFunc branch from 7d3b849 to 96579ed Compare March 22, 2022 03:22
@sethvargo sethvargo merged commit 95fc73d into GoogleCloudPlatform:main Mar 22, 2022
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.

None yet

2 participants