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

Synchronise access to the map when printing. #1406

Merged
merged 2 commits into from
Nov 15, 2022

Conversation

felixdesouza
Copy link
Contributor

@felixdesouza felixdesouza commented Nov 14, 2022

If you happen to run:

docker buildx imagetools inspect --format '{{. | json}}' imageRef

Where imageRef points to a manifest list with multiple platforms.

You can sometimes run into concurrent map write errors as there are multiple goroutines trying to write to the imageInfo and buildInfo maps.

NB: I'm not a Go developer, if there is a nicer way to do this, happy to make edits.

@jedevc
Copy link
Collaborator

jedevc commented Nov 14, 2022

Hey, nice find! 🎉 Thanks for the PR as well ❤️

Ideally we should change the map to an array, since then we can assign to it in parallel, using the index of the loop as the index into the resulting array (similar to how we do it in exporter/containerimage/writer.go). But the logic here seems a little fiddly, so I think we should just be able to have locks.

With that, a couple notes:

  • We only need sync.Mutex (instead of sync.RWMutex)
  • We don't need to take the address of with &
  • Ideally, we could have two mutexes, one for each map that we write to

CC @crazy-max, this was added in #972, should we cherry-pick? Probably to 0.9 at least?

@felixdesouza
Copy link
Contributor Author

Thanks for the review!

@jedevc
Copy link
Collaborator

jedevc commented Nov 14, 2022

@felixdesouza could you sign-off your second commit as well / squash into the first?

Signed-off-by: Felix de Souza <[email protected]>
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.

3 participants