progress: fix sync issue in controller. #2641
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A panic was encountered where writes to progress status hit a nil writer. While not easy to reproduce, this commit fixes a likely culprit. The use of atomics in the controller was not safe against a race such as the following: 1. One goroutine calls Start, incrementing count but not yet setting writer. 2. A second goroutine calls Start, increments count again but sees that count >1 and thus doesn't try to set writer. 3. The second goroutine then calls Status, assuming the writer has been set, but the first goroutine still hasn't set the writer yet, causing a nil pointer exception. This commit fixes that issue by just using a mutex instead of atomics. It also adds a nil check for the writer just to be safe against panics due to unknown issues in the future as missing a status update is much better than buildkitd crashing. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
tonistiigi
approved these changes
Feb 16, 2022
|
Will this be included in v0.10.0-rc2? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A panic was encountered where writes to progress status hit a nil
writer. While not easy to reproduce, this commit fixes a likely culprit.
The use of atomics in the controller was not safe against a race such as
the following:
writer.
count >1 and thus doesn't try to set writer.
set, but the first goroutine still hasn't set the writer yet, causing
a nil pointer exception.
This commit fixes that issue by just using a mutex instead of atomics.
It also adds a nil check for the writer just to be safe against panics
due to unknown issues in the future as missing a status update is much
better than buildkitd crashing.
Signed-off-by: Erik Sipsma erik@sipsma.dev
This address the panic mentioned in the comment here, though the problem is unrelated to that PR: #2611 (comment)