Changeset 31669 for main/trunk

Timestamp:

2017-05-10T18:04:08+12:00 (7 years ago)

Author:

ak19

Message:

Have made the EventListeners list in CollectionConstructor use the threadsafe if less efficient CopyOnWriteArrayList, so that the send event messages called by GS2PerlConstructor are threadsafe. This reduces the need to make certain methods in GS2PerlConstructor declared as synchronized, which would have locked the GS2PerlConstructor object instance rather than its inherited listeners member variable.

Location:

main/trunk/greenstone3/src/java/org/greenstone/gsdl3/build

Files:

• CollectionConstructor.java (modified) (4 diffs)
• GS2PerlConstructor.java (modified) (3 diffs)

main/trunk/greenstone3/src/java/org/greenstone/gsdl3/build/CollectionConstructor.java

@@ -5 +5 @@
 import org.greenstone.gsdl3.util.*;
 //import java.util.Thread
-import javax.swing.event.EventListenerList;
+import java.util.concurrent.CopyOnWriteArrayList;
 import org.w3c.dom.Element;
 
@@ -22 +22 @@
     /** other arguments/parameters for the construction process - in a paramList */
     protected Element process_params = null;
-    /** the list of listeners for the process */
-    protected EventListenerList listeners = null;
+    /** the list of listeners for the process. We need it to be threadsafe. 
+     * see http://stackoverflow.com/questions/8259479/should-i-synchronize-listener-notifications-or-not
+     * https://docs.oracle.com/javase/6/docs/api/java/util/concurrent/CopyOnWriteArrayList.html
+     * "A thread-safe variant of ArrayList in which all mutative operations (add, set, and so on) are
+     * implemented by making a fresh copy of the underlying array.
+     * This is ordinarily too costly, but may be more efficient than alternatives when traversal operations
+     * vastly outnumber mutations, and is useful when you cannot or don't want to synchronize traversals,
+     * yet need to preclude interference among concurrent threads."
+     */
+    protected final CopyOnWriteArrayList<ConstructionListener> listeners;
     /** A flag used to determine if this process has been asked to cancel. */
     protected boolean cancel = false; // Not really used (in any way that works)
@@ -34 +42 @@
     {
         super(name);
-        this.listeners = new EventListenerList();
+        this.listeners = new CopyOnWriteArrayList<ConstructionListener>();
     }
 
@@ -87 +95 @@
     public boolean addListener(ConstructionListener listener)
     {
-        this.listeners.add(ConstructionListener.class, listener);
-        return true;
+        this.listeners.add(listener);
+        return true;
     }
 
     public boolean removeListener(ConstructionListener listener)
-    {
-        this.listeners.remove(ConstructionListener.class, listener);
-        return true;
+    {       
+        this.listeners.remove(listener);
+        return true;
     }
 
     protected void sendProcessBegun(ConstructionEvent evt)
     {
-        Object[] concerned = this.listeners.getListenerList();
-        for (int i = 0; i < concerned.length; i += 2)
-        {
-            if (concerned[i] == ConstructionListener.class)
-            {
-                ((ConstructionListener) concerned[i + 1]).processBegun(evt);
-            }
-        }
+        // See http://stackoverflow.com/questions/8259479/should-i-synchronize-listener-notifications-or-not
+        for (ConstructionListener l: this.listeners) {
+        l.processBegun(evt);
+        }
     }
 
     protected void sendProcessComplete(ConstructionEvent evt)
     {
-        Object[] concerned = this.listeners.getListenerList();
-        for (int i = 0; i < concerned.length; i += 2)
-        {
-            if (concerned[i] == ConstructionListener.class)
-            {
-                ((ConstructionListener) concerned[i + 1]).processComplete(evt);
-            }
-        }
+        for (ConstructionListener l: this.listeners) {
+        l.processComplete(evt);
+        }
     }
 
-    protected synchronized void sendProcessStatus(ConstructionEvent evt)
+    // Method doesn't need to be synchronized any more, since it uses the ThreadSafe CopyOnWriteArrayList
+    // for listeners list.
+    // See http://stackoverflow.com/questions/8259479/should-i-synchronize-listener-notifications-or-not
+    // See http://stackoverflow.com/questions/574240/is-there-an-advantage-to-use-a-synchronized-method-instead-of-a-synchronized-blo
+    protected void sendProcessStatus(ConstructionEvent evt)
     {
-
-        Object[] concerned = this.listeners.getListenerList();
-        for (int i = 0; i < concerned.length; i += 2)
-        {
-            if (concerned[i] == ConstructionListener.class)
-            {
-                ((ConstructionListener) concerned[i + 1]).processStatus(evt);
-            }
-        }
+        for (ConstructionListener l: this.listeners) {
+        l.processStatus(evt);
+        }
     }
 
     protected void sendMessage(ConstructionEvent evt)
     {
-
-        Object[] concerned = this.listeners.getListenerList();
-        for (int i = 0; i < concerned.length; i += 2)
-        {
-            if (concerned[i] == ConstructionListener.class)
-            {
-                ((ConstructionListener) concerned[i + 1]).message(evt);
-            }
-        }
+        for (ConstructionListener l: this.listeners) {
+        l.message(evt);
+        }
     }
 

main/trunk/greenstone3/src/java/org/greenstone/gsdl3/build/GS2PerlConstructor.java

@@ -449 +449 @@
     }
 
-        
+
     protected boolean runPerlCommand(String[] command, String[] envvars, File dir)
     {
@@ -700 +700 @@
     // exceptions when reading from our perl process' stderr and stdout streams are handled by
     // SynchronizedProcessHandler.gotException() below, since they happen in separate threads
-    // from this one (the one from which the perl process is run).
-    public synchronized void gotException(Exception e) {
+    // from this one (the one from which the perl process is run). So this method's operations
+    // have been made thread-safe, rather than synchronizing on the method itself which locks
+    // *this* object and which isn't actually useful for what I want to do.
+    public void gotException(Exception e) {
 
     // do what original runPerlCommand() code always did when an exception occurred
@@ -791 +793 @@
 
         // this next method is thread safe since only synchronized methods are invoked.
-        // and only immutable (final) vars are used. NO, What about the listeners???
-        // So have made this next method synchronized, which synchronizes on the GS2PerlConstructor
-        // and consequently its member variables are protected against concurrent access
-        // by multiple threads.
+        // and only immutable (final) vars are used. And the listeners of this class have
+        // now been made threadsafe by using CopyOnWriteArrayList in place of EventListenerList.
         sendProcessStatus(new ConstructionEvent(GS2PerlConstructor.this, GSStatus.CONTINUING, line));