From 0924c45dd84be115c6c622b075445855b13951e5 Mon Sep 17 00:00:00 2001 From: Niki Roo Date: Thu, 9 Mar 2017 08:10:28 +0100 Subject: [PATCH] Progress: fix synchro issues + tests --- src/be/nikiroo/utils/Progress.java | 117 ++++++++++++++------ src/be/nikiroo/utils/test/ProgressTest.java | 74 +++++++++++++ 2 files changed, 158 insertions(+), 33 deletions(-) diff --git a/src/be/nikiroo/utils/Progress.java b/src/be/nikiroo/utils/Progress.java index 62e4687..5af7b30 100644 --- a/src/be/nikiroo/utils/Progress.java +++ b/src/be/nikiroo/utils/Progress.java @@ -30,6 +30,8 @@ public class Progress { public void progress(Progress progress, String name); } + private Progress parent = null; + private Object lock = new Object(); private String name; private Map children; private List listeners; @@ -122,18 +124,24 @@ public class Progress { * * @param min * the min to set + * + * + * @throws Error + * if min < 0 or if min > max */ public void setMin(int min) { if (min < 0) { throw new Error("negative values not supported"); } - if (min > max) { - throw new Error( - "The minimum progress value must be <= the maximum progress value"); - } + synchronized (getLock()) { + if (min > max) { + throw new Error( + "The minimum progress value must be <= the maximum progress value"); + } - this.min = min; + this.min = min; + } } /** @@ -150,14 +158,20 @@ public class Progress { * * @param max * the max to set + * + * + * @throws Error + * if max < min */ public void setMax(int max) { - if (max < min) { - throw new Error( - "The maximum progress value must be >= the minimum progress value"); - } + synchronized (getLock()) { + if (max < min) { + throw new Error( + "The maximum progress value must be >= the minimum progress value"); + } - this.max = max; + this.max = max; + } } /** @@ -167,6 +181,9 @@ public class Progress { * the min * @param max * the max + * + * @throws Error + * if min < 0 or if min > max */ public void setMinMax(int min, int max) { if (min < 0) { @@ -178,8 +195,10 @@ public class Progress { "The minimum progress value must be <= the maximum progress value"); } - this.min = min; - this.max = max; + synchronized (getLock()) { + this.min = min; + this.max = max; + } } /** @@ -202,9 +221,11 @@ public class Progress { * the progress to set */ public void setProgress(int progress) { - int diff = this.progress - this.localProgress; - this.localProgress = progress; - setTotalProgress(this, name, progress + diff); + synchronized (getLock()) { + int diff = this.progress - this.localProgress; + this.localProgress = progress; + setTotalProgress(this, name, progress + diff); + } } /** @@ -251,10 +272,12 @@ public class Progress { * the progress to set */ private void setTotalProgress(Progress pg, String name, int progress) { - this.progress = progress; + synchronized (getLock()) { + this.progress = progress; - for (ProgressListener l : listeners) { - l.progress(pg, name); + for (ProgressListener l : listeners) { + l.progress(pg, name); + } } } @@ -293,32 +316,60 @@ public class Progress { * the weight (on a {@link Progress#getMin()} to * {@link Progress#getMax()} scale) of this child * {@link Progress} in relation to its parent + * + * @throws Error + * if weight exceed {@link Progress#getMax()} or if progress + * already has a parent */ public void addProgress(Progress progress, double weight) { if (weight < min || weight > max) { - throw new Error( - "A Progress object cannot have a weight outside its parent range"); + throw new Error(String.format( + "Progress object %s cannot have a weight of %f, " + + "it is outside of its parent (%s) range (%f)", + progress.name, weight, name, max)); + } + + if (progress.parent != null) { + throw new Error(String.format( + "Progress object %s cannot be added to %s, " + + "as it already has a parent (%s)", progress.name, + name, progress.parent.name)); } - // Note: this is quite inefficient, especially with many children - // TODO: improve it? progress.addProgressListener(new ProgressListener() { public void progress(Progress pg, String name) { - double total = ((double) localProgress) / (max - min); - for (Entry entry : children.entrySet()) { - total += (entry.getValue() / (max - min)) - * entry.getKey().getRelativeProgress(); - } - - if (name == null) { - name = Progress.this.name; + synchronized (getLock()) { + double total = ((double) localProgress) / (max - min); + for (Entry entry : children.entrySet()) { + total += (entry.getValue() / (max - min)) + * entry.getKey().getRelativeProgress(); + } + + if (name == null) { + name = Progress.this.name; + } + + setTotalProgress(pg, name, + (int) Math.round(total * (max - min))); } - - setTotalProgress(pg, name, - (int) Math.round(total * (max - min))); } }); this.children.put(progress, weight); } + + /** + * The lock object to use (this one or the recursively-parent one). + * + * @return the lock object to use + */ + private Object getLock() { + synchronized (lock) { + if (parent != null) { + return parent.getLock(); + } + + return lock; + } + } } diff --git a/src/be/nikiroo/utils/test/ProgressTest.java b/src/be/nikiroo/utils/test/ProgressTest.java index c697973..be8b46a 100644 --- a/src/be/nikiroo/utils/test/ProgressTest.java +++ b/src/be/nikiroo/utils/test/ProgressTest.java @@ -181,6 +181,80 @@ class ProgressTest extends TestLauncher { assertEquals(1000, pg); } }); + + addTest(new TestCase("Listeners with children, multi-thread") { + int pg; + boolean decrease; + Object lock1 = new Object(); + Object lock2 = new Object(); + int currentStep1; + int currentStep2; + + @Override + public void test() throws Exception { + final Progress p = new Progress(0, 200); + + final Progress child1 = new Progress(); + final Progress child2 = new Progress(); + p.addProgress(child1, 100); + p.addProgress(child2, 100); + + p.addProgressListener(new Progress.ProgressListener() { + public void progress(Progress progress, String name) { + int now = p.getProgress(); + if (now < pg) { + decrease = true; + } + pg = now; + } + }); + + // Run 200 concurrent threads, 2 at a time allowed to + // make progress (each on a different child) + for (int i = 0; i <= 100; i++) { + final int step = i; + new Thread(new Runnable() { + public void run() { + synchronized (lock1) { + if (step > currentStep1) { + currentStep1 = step; + child1.setProgress(step); + } + } + } + }).start(); + + new Thread(new Runnable() { + public void run() { + synchronized (lock2) { + if (step > currentStep2) { + currentStep2 = step; + child2.setProgress(step); + } + } + } + }).start(); + } + + int i; + int timeout = 20; // in 1/10th of seconds + for (i = 0; i < timeout + && (currentStep1 + currentStep2) < 200; i++) { + Thread.sleep(100); + } + + assertEquals("The test froze at step " + currentStep1 + + " + " + currentStep2, true, i < timeout); + assertEquals( + "There should not have any decresing steps", + decrease, false); + assertEquals("The progress should have reached 200", + 200, p.getProgress()); + assertEquals( + "The progress should have reached completion", + true, p.isDone()); + } + }); } }); } -- 2.27.0