-
Notifications
You must be signed in to change notification settings - Fork 703
Fix WaitGroup usage #581
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
Fix WaitGroup usage #581
Conversation
I propose this instead of #577, @gabemontero @bparees PTAL. |
pkg/build/strategies/sti/sti.go
Outdated
} | ||
|
||
wg := sync.WaitGroup{} | ||
var wg sync.WaitGroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to initialize WaitGroups, just declare them and they are ready for use.
Idiomatic example: https://golang.org/pkg/sync/#example_WaitGroup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't let the PM team see this :-) ... they prefer the :=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i also prefer := :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a necessary change, so will revert it to minimize the patch ;)
LGTM ... I'll delete the other pull when this merges. @bparees - shall we assign the associated bugzilla and UPSTREAM vendor update to @rhcarvalho :-) ? |
Would be fun, but I don't want to overcommit :) I'll be happy to send more s2i PRs when I come back (soon) 😄 , missing you guys |
Calling wg.Done() does not "cancel" the WaitGroup, it does introduce a bug in which depending on timing the WaitGroup counter might go negative, what should never happen. Would be nice to rewrite the method to be correct concurrent code, but for now should be enough to avoid the panic condition and note that there is a potential leak. --- Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1370265
0b63dd2
to
b5d4d3e
Compare
lgtm [merge] |
Evaluated for source to image merge up to b5d4d3e |
Source To Image Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_sti/207/) |
roger that @bparees |
and thx @rhcarvalho :-) |
Calling wg.Done() does not "cancel" the WaitGroup, it does introduce a
bug in which depending on timing the WaitGroup counter might go
negative, what should never happen.
Would be nice to rewrite the method to be correct concurrent code, but
for now should be enough to avoid the panic condition and note that
there is a potential leak.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1370265