Progress: fix synchro issues + tests

diff --git a/src/be/nikiroo/utils/Progress.java b/src/be/nikiroo/utils/Progress.java
index 62e46874c3e0918ff49877f74b4d7314bae0be83..5af7b30d83b357654d2b34e3bff3faa3eb8de9d6 100644 (file)
--- 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);
        }
 
                public void progress(Progress progress, String name);
        }
 

+       private Progress parent = null;
+       private Object lock = new Object();

        private String name;
        private Map<Progress, Double> children;
        private List<ProgressListener> listeners;
        private String name;
        private Map<Progress, Double> children;
        private List<ProgressListener> listeners;


@@ -122,18 +124,24 @@ public class Progress {
         * 
         * @param min
         *            the min to set
         * 
         * @param min
         *            the min to set

+        * 
+        * 
+        * @throws Error
+        *             if min &lt; 0 or if min &gt; max

         */
        public void setMin(int min) {
                if (min < 0) {
                        throw new Error("negative values not supported");
                }
 
         */
        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
         * 
         * @param max
         *            the max to set

+        * 
+        * 
+        * @throws Error
+        *             if max &lt; min

         */
        public void setMax(int max) {
         */
        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
         *            the min
         * @param max
         *            the max

+        * 
+        * @throws Error
+        *             if min &lt; 0 or if min &gt; max

         */
        public void setMinMax(int min, int max) {
                if (min < 0) {
         */
        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");
                }
 
                                        "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) {
         *            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) {
         *            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
         *            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) {
         */
        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) {
                progress.addProgressListener(new ProgressListener() {
                        public void progress(Progress pg, String name) {

-                               double total = ((double) localProgress) / (max - min);
-                               for (Entry<Progress, Double> 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<Progress, Double> 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);
        }
                        }
                });
 
                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 c6979739a1ac19fa98e065934170daa3b0c3ea3e..be8b46a0103ac2d9b6741d2d3741b40a55362951 100644 (file)
--- 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);
                                        }
                                });
                                                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());
+                                       }
+                               });

                        }
                });
        }
                        }
                });
        }