From b5d4d3e8aa485e2cb1ebb027da8c03f1a469be3a Mon Sep 17 00:00:00 2001 From: Rodolfo Carvalho Date: Thu, 1 Sep 2016 19:38:49 +0200 Subject: [PATCH] Fix WaitGroup usage 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 --- pkg/build/strategies/sti/sti.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/build/strategies/sti/sti.go b/pkg/build/strategies/sti/sti.go index ba5b1eda3..7c0c30ecd 100644 --- a/pkg/build/strategies/sti/sti.go +++ b/pkg/build/strategies/sti/sti.go @@ -560,7 +560,6 @@ func (builder *STI) Execute(command string, user string, config *api.Config) err }() opts.Stdin = r - defer wg.Wait() } go func(reader io.Reader) { @@ -591,16 +590,17 @@ func (builder *STI) Execute(command string, user string, config *api.Config) err go dockerpkg.StreamContainerIO(errReader, &errOutput, func(a ...interface{}) { glog.Info(a...) }) err := builder.docker.RunContainer(opts) - if util.IsTimeoutError(err) { - // Cancel waiting for source input if the container timeouts - wg.Done() - } if e, ok := err.(errors.ContainerError); ok { // even with deferred close above, close errReader now so we avoid data race condition on errOutput; // closing will cause StreamContainerIO to exit, thus releasing the writer in the equation errReader.Close() return errors.NewContainerError(config.BuilderImage, e.ErrorCode, errOutput) } + // Do not wait for source input if the container times out. + // FIXME: this potentially leaks a goroutine. + if !util.IsTimeoutError(err) { + wg.Wait() + } return err }