From efe3b61e15149616951531342297f1934c9c908e Mon Sep 17 00:00:00 2001 From: Jonathan Boulle Date: Sun, 29 May 2016 14:32:53 +0200 Subject: [PATCH 1/3] progressutil: fix setting of copyReader.done By setting the value during the Read call, we potentially miss errors in the CopyProgressPrinter's main loop, because the copyReader might have not yet propagated them before it marks itself as done. --- progressutil/iocopy.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/progressutil/iocopy.go b/progressutil/iocopy.go index d0572de..70be3bf 100644 --- a/progressutil/iocopy.go +++ b/progressutil/iocopy.go @@ -50,9 +50,6 @@ func (cr *copyReader) Read(p []byte) (int, error) { if err == nil { err = err1 } - if err != nil { - cr.setDone(true) - } return n, err } @@ -105,6 +102,7 @@ func (cpp *CopyProgressPrinter) AddCopy(reader io.Reader, name string, size int6 cpp.errors = append(cpp.errors, err) cpp.lock.Unlock() } + cr.setDone(true) }() } From 77313bbcdf9c9b5ec57986fd9c354d97dd113665 Mon Sep 17 00:00:00 2001 From: Jonathan Boulle Date: Sun, 29 May 2016 14:34:50 +0200 Subject: [PATCH 2/3] progressutil: remove extraneous else --- progressutil/iocopy.go | 1 - 1 file changed, 1 deletion(-) diff --git a/progressutil/iocopy.go b/progressutil/iocopy.go index 70be3bf..a335545 100644 --- a/progressutil/iocopy.go +++ b/progressutil/iocopy.go @@ -135,7 +135,6 @@ func (cpp *CopyProgressPrinter) PrintAndWait(printTo io.Writer, printInterval ti if err != nil { return err } - } else { } allDone := true From f24e4f6a55a09ae816d44b6ed1c1e09615ffcc73 Mon Sep 17 00:00:00 2001 From: Jonathan Boulle Date: Sun, 29 May 2016 14:36:26 +0200 Subject: [PATCH 3/3] progressutil: check for errors again when done There's a race between the time we first check the error slice and when we check for allDone, since a copyReader might finish and try to report an error anywhere between this loop. Correct this by checking for errors again before returning. This is a stopgap - the real fix here is to refactor this a little more invasively. --- progressutil/iocopy.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/progressutil/iocopy.go b/progressutil/iocopy.go index a335545..d41bce2 100644 --- a/progressutil/iocopy.go +++ b/progressutil/iocopy.go @@ -142,6 +142,15 @@ func (cpp *CopyProgressPrinter) PrintAndWait(printTo io.Writer, printInterval ti allDone = allDone && r.getDone() } if allDone && len(readers) > 0 { + // We still need to check for errors again, as one may + // have occurred between when we first checked and when + // the readers were marked as done + cpp.lock.Lock() + errors = cpp.errors + cpp.lock.Unlock() + if len(errors) > 0 { + return errors[0] + } return nil }