Changeset 31669

Show
Ignore:
Timestamp:
10.05.2017 18:04:08 (2 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:
2 modified

Legend:

Unmodified
Added
Removed
  • main/trunk/greenstone3/src/java/org/greenstone/gsdl3/build/CollectionConstructor.java

    r31667 r31669  
    55import org.greenstone.gsdl3.util.*; 
    66//import java.util.Thread 
    7 import javax.swing.event.EventListenerList; 
     7import java.util.concurrent.CopyOnWriteArrayList; 
    88import org.w3c.dom.Element; 
    99 
     
    2222    /** other arguments/parameters for the construction process - in a paramList */ 
    2323    protected Element process_params = null; 
    24     /** the list of listeners for the process */ 
    25     protected EventListenerList listeners = null; 
     24    /** the list of listeners for the process. We need it to be threadsafe.  
     25     * see http://stackoverflow.com/questions/8259479/should-i-synchronize-listener-notifications-or-not 
     26     * https://docs.oracle.com/javase/6/docs/api/java/util/concurrent/CopyOnWriteArrayList.html 
     27     * "A thread-safe variant of ArrayList in which all mutative operations (add, set, and so on) are 
     28     * implemented by making a fresh copy of the underlying array. 
     29     * This is ordinarily too costly, but may be more efficient than alternatives when traversal operations 
     30     * vastly outnumber mutations, and is useful when you cannot or don't want to synchronize traversals, 
     31     * yet need to preclude interference among concurrent threads." 
     32     */ 
     33    protected final CopyOnWriteArrayList<ConstructionListener> listeners; 
    2634    /** A flag used to determine if this process has been asked to cancel. */ 
    2735    protected boolean cancel = false; // Not really used (in any way that works) 
     
    3442    { 
    3543        super(name); 
    36         this.listeners = new EventListenerList(); 
     44        this.listeners = new CopyOnWriteArrayList<ConstructionListener>(); 
    3745    } 
    3846 
     
    8795    public boolean addListener(ConstructionListener listener) 
    8896    { 
    89         this.listeners.add(ConstructionListener.class, listener); 
    90         return true; 
     97        this.listeners.add(listener); 
     98        return true; 
    9199    } 
    92100 
    93101    public boolean removeListener(ConstructionListener listener) 
    94     { 
    95         this.listeners.remove(ConstructionListener.class, listener); 
    96         return true; 
     102    {        
     103        this.listeners.remove(listener); 
     104        return true; 
    97105    } 
    98106 
    99107    protected void sendProcessBegun(ConstructionEvent evt) 
    100108    { 
    101         Object[] concerned = this.listeners.getListenerList(); 
    102         for (int i = 0; i < concerned.length; i += 2) 
    103         { 
    104             if (concerned[i] == ConstructionListener.class) 
    105             { 
    106                 ((ConstructionListener) concerned[i + 1]).processBegun(evt); 
    107             } 
    108         } 
     109        // See http://stackoverflow.com/questions/8259479/should-i-synchronize-listener-notifications-or-not 
     110        for (ConstructionListener l: this.listeners) { 
     111        l.processBegun(evt); 
     112        } 
    109113    } 
    110114 
    111115    protected void sendProcessComplete(ConstructionEvent evt) 
    112116    { 
    113         Object[] concerned = this.listeners.getListenerList(); 
    114         for (int i = 0; i < concerned.length; i += 2) 
    115         { 
    116             if (concerned[i] == ConstructionListener.class) 
    117             { 
    118                 ((ConstructionListener) concerned[i + 1]).processComplete(evt); 
    119             } 
    120         } 
     117        for (ConstructionListener l: this.listeners) { 
     118        l.processComplete(evt); 
     119        } 
    121120    } 
    122121 
    123     protected synchronized void sendProcessStatus(ConstructionEvent evt) 
     122    // Method doesn't need to be synchronized any more, since it uses the ThreadSafe CopyOnWriteArrayList 
     123    // for listeners list. 
     124    // See http://stackoverflow.com/questions/8259479/should-i-synchronize-listener-notifications-or-not 
     125    // See http://stackoverflow.com/questions/574240/is-there-an-advantage-to-use-a-synchronized-method-instead-of-a-synchronized-blo 
     126    protected void sendProcessStatus(ConstructionEvent evt) 
    124127    { 
    125  
    126         Object[] concerned = this.listeners.getListenerList(); 
    127         for (int i = 0; i < concerned.length; i += 2) 
    128         { 
    129             if (concerned[i] == ConstructionListener.class) 
    130             { 
    131                 ((ConstructionListener) concerned[i + 1]).processStatus(evt); 
    132             } 
    133         } 
     128        for (ConstructionListener l: this.listeners) { 
     129        l.processStatus(evt); 
     130        } 
    134131    } 
    135132 
    136133    protected void sendMessage(ConstructionEvent evt) 
    137134    { 
    138  
    139         Object[] concerned = this.listeners.getListenerList(); 
    140         for (int i = 0; i < concerned.length; i += 2) 
    141         { 
    142             if (concerned[i] == ConstructionListener.class) 
    143             { 
    144                 ((ConstructionListener) concerned[i + 1]).message(evt); 
    145             } 
    146         } 
     135        for (ConstructionListener l: this.listeners) { 
     136        l.message(evt); 
     137        } 
    147138    } 
    148139 
  • main/trunk/greenstone3/src/java/org/greenstone/gsdl3/build/GS2PerlConstructor.java

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