From cf2c309c5e28b0f6e37c284916c1b698e12a7edf Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 15 Oct 2018 13:53:51 +0200 Subject: [PATCH 1/5] add race conditions triggers for progress bar --- packer/progressbar_test.go | 58 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 packer/progressbar_test.go diff --git a/packer/progressbar_test.go b/packer/progressbar_test.go new file mode 100644 index 000000000..89cbe862a --- /dev/null +++ b/packer/progressbar_test.go @@ -0,0 +1,58 @@ +package packer + +import ( + "sync" + "testing" +) + +func TestStackableProgressBar_race(t *testing.T) { + bar := &StackableProgressBar{} + + start42Fn := func() { bar.Start(42) } + finishFn := func() { bar.Finish() } + add21 := func() { bar.Add(21) } + // prefix := func() { bar.prefix() } + + type fields struct { + pre func() + calls []func() + post func() + } + tests := []struct { + name string + fields fields + iterations int + }{ + {"all public", fields{nil, []func(){start42Fn, finishFn, add21, add21}, finishFn}, 300}, + {"all public", fields{start42Fn, []func(){add21, add21}, finishFn}, 300}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + for i := 0; i < tt.iterations; i++ { + if tt.fields.pre != nil { + tt.fields.pre() + } + var startWg, endWg sync.WaitGroup + startWg.Add(1) + endWg.Add(len(tt.fields.calls)) + for _, call := range tt.fields.calls { + call := call + go func() { + defer endWg.Done() + startWg.Wait() + call() + }() + } + startWg.Done() // everyone is initialized, let's unlock everyone at the same time. + // .... + endWg.Wait() // wait for all calls to return. + if tt.fields.post != nil { + tt.fields.post() + } + } + }) + } + +} From 56ccba86c7622333311e5fcb243815c9f3ddcaa3 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 15 Oct 2018 13:53:59 +0200 Subject: [PATCH 2/5] fix some race conditions --- packer/progressbar.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packer/progressbar.go b/packer/progressbar.go index f08928767..fed58f41a 100644 --- a/packer/progressbar.go +++ b/packer/progressbar.go @@ -66,7 +66,9 @@ func (spb *StackableProgressBar) Start(total int64) { func (spb *StackableProgressBar) Add(total int64) { spb.mtx.Lock() defer spb.mtx.Unlock() - spb.Bar.Add(total) + if spb.Bar.ProgressBar != nil { + spb.Bar.Add(total) + } } func (spb *StackableProgressBar) NewProxyReader(r io.Reader) io.Reader { @@ -87,12 +89,13 @@ func (spb *StackableProgressBar) Finish() { if spb.items == 0 { // slef cleanup spb.Bar.ProgressBar.Finish() - spb.Bar.ProgressBar = nil spb.started = false spb.total = 0 return } - spb.prefix() + if spb.Bar.ProgressBar != nil { + spb.prefix() + } } // BasicProgressBar is packer's basic progress bar. From 4cae413a29f1a8032a3d67f1f383b3f4e25c6dd3 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 15 Oct 2018 15:28:08 +0200 Subject: [PATCH 3/5] remove unedded atomic call ( protected by a lock ) --- packer/progressbar.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packer/progressbar.go b/packer/progressbar.go index fed58f41a..3052d3ee0 100644 --- a/packer/progressbar.go +++ b/packer/progressbar.go @@ -4,7 +4,6 @@ import ( "fmt" "io" "sync" - "sync/atomic" "github.com/cheggaaa/pb" ) @@ -78,7 +77,7 @@ func (spb *StackableProgressBar) NewProxyReader(r io.Reader) io.Reader { } func (spb *StackableProgressBar) prefix() { - spb.Bar.ProgressBar.Prefix(fmt.Sprintf("%d items: ", atomic.LoadInt32(&spb.items))) + spb.Bar.ProgressBar.Prefix(fmt.Sprintf("%d items: ", spb.items)) } func (spb *StackableProgressBar) Finish() { From 27f4b9e4a1ec9537cfa4006879693bdf3a37dee4 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 15 Oct 2018 15:28:46 +0200 Subject: [PATCH 4/5] pb/testing: triger more race condition by having the progress bar refresh more often --- packer/progressbar.go | 19 ++++++++++++++----- packer/progressbar_test.go | 15 +++++++++++++-- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/packer/progressbar.go b/packer/progressbar.go index 3052d3ee0..8f1d3d9a5 100644 --- a/packer/progressbar.go +++ b/packer/progressbar.go @@ -35,16 +35,25 @@ type StackableProgressBar struct { items int32 total int64 - started bool + started bool + ConfigProgressbarFN func(*pb.ProgressBar) } var _ ProgressBar = new(StackableProgressBar) -func (spb *StackableProgressBar) start() { - spb.Bar.ProgressBar = pb.New(0) - spb.Bar.ProgressBar.SetUnits(pb.U_BYTES) +func defaultProgressbarConfigFn(bar *pb.ProgressBar) { + bar.SetUnits(pb.U_BYTES) +} - spb.Bar.ProgressBar.Start() +func (spb *StackableProgressBar) start() { + bar := pb.New(0) + if spb.ConfigProgressbarFN == nil { + spb.ConfigProgressbarFN = defaultProgressbarConfigFn + } + spb.ConfigProgressbarFN(bar) + + bar.Start() + spb.Bar.ProgressBar = bar spb.started = true } diff --git a/packer/progressbar_test.go b/packer/progressbar_test.go index 89cbe862a..db75ad06b 100644 --- a/packer/progressbar_test.go +++ b/packer/progressbar_test.go @@ -3,10 +3,21 @@ package packer import ( "sync" "testing" + "time" + + "github.com/cheggaaa/pb" ) +func speedyProgressBar(bar *pb.ProgressBar) { + bar.SetUnits(pb.U_BYTES) + bar.SetRefreshRate(1 * time.Millisecond) + bar.NotPrint = true +} + func TestStackableProgressBar_race(t *testing.T) { - bar := &StackableProgressBar{} + bar := &StackableProgressBar{ + ConfigProgressbarFN: speedyProgressBar, + } start42Fn := func() { bar.Start(42) } finishFn := func() { bar.Finish() } @@ -24,7 +35,7 @@ func TestStackableProgressBar_race(t *testing.T) { iterations int }{ {"all public", fields{nil, []func(){start42Fn, finishFn, add21, add21}, finishFn}, 300}, - {"all public", fields{start42Fn, []func(){add21, add21}, finishFn}, 300}, + {"add", fields{start42Fn, []func(){add21}, finishFn}, 300}, } for _, tt := range tests { From 25775cd2667a601e20719ba7642851e14ed17442 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 15 Oct 2018 16:31:59 +0200 Subject: [PATCH 5/5] fix more race conditions --- packer/progressbar.go | 10 +++++----- packer/progressbar_test.go | 2 ++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/packer/progressbar.go b/packer/progressbar.go index 8f1d3d9a5..e7405b074 100644 --- a/packer/progressbar.go +++ b/packer/progressbar.go @@ -93,17 +93,17 @@ func (spb *StackableProgressBar) Finish() { spb.mtx.Lock() defer spb.mtx.Unlock() - spb.items-- - if spb.items == 0 { + if spb.items < 0 { + spb.items-- + } + if spb.items == 0 && spb.Bar.ProgressBar != nil { // slef cleanup spb.Bar.ProgressBar.Finish() + spb.Bar.ProgressBar = nil spb.started = false spb.total = 0 return } - if spb.Bar.ProgressBar != nil { - spb.prefix() - } } // BasicProgressBar is packer's basic progress bar. diff --git a/packer/progressbar_test.go b/packer/progressbar_test.go index db75ad06b..d19827989 100644 --- a/packer/progressbar_test.go +++ b/packer/progressbar_test.go @@ -12,6 +12,7 @@ func speedyProgressBar(bar *pb.ProgressBar) { bar.SetUnits(pb.U_BYTES) bar.SetRefreshRate(1 * time.Millisecond) bar.NotPrint = true + bar.Format("[\x00=\x00>\x00-\x00]") } func TestStackableProgressBar_race(t *testing.T) { @@ -36,6 +37,7 @@ func TestStackableProgressBar_race(t *testing.T) { }{ {"all public", fields{nil, []func(){start42Fn, finishFn, add21, add21}, finishFn}, 300}, {"add", fields{start42Fn, []func(){add21}, finishFn}, 300}, + {"add start", fields{start42Fn, []func(){start42Fn, add21}, finishFn}, 300}, } for _, tt := range tests {