|
Hi,
A CopyOnWriteArrayList is used to hold the layers of the MapContent, which seems a pretty good idea to resolve concurrency problems around this list.
The thing is, you don't need anymore in the methods MapContent.xxx() with xxx a method accessing/modifying this list (i.e 'addLayer()', 'removeLayer()', ... and most important of all 'layers()') to manage the 'monitor' lock ! Because CopyOnWriteArrayList is already multithreads safe.
We could think this is of no importance, but... This can ('did' for me...) engage some deadlocks, working with this methods. (I have found a workaround solution (we always can find one...), but still, since this locks really are useless, I think it's better to remove them).
-------------
Example in a multithreaded environment:
Thread X is a background thread.
Thread Y is the AWT thread, in charge of rendering Swing components.
The component in charge of drawing is a subclass of AbstractMapPane, using the AbstractMapPane.drawingLock when painting. (Remember AbstractMapPane is a JPanel, which content is drawn in JComponent.paintComponent()).
1) Thread X takes the lock, in order to remove a layer. (why not ?) So here we are at the beginning of MapContent.remove() function, the 'monitor' lock has been taken by Thread X.
2) Thread Y is called to paint the AbstractMapPane, so it takes the lock 'drawingLock' of AbstractMapPane when entering the 'paintComponent' method.
Imagine, Thread Y, in order to repaint, calls MapContent.layers() in order to have an access to the layers list of the MapContent. (why not ?)
So here we are, Thread Y have got the AbstractMapPane.drawingLock, and Thread X have got MapContent.monitor lock
For now, Thread Y is waiting, to be able to lock MapContent.monitor which has uselessly been taken by Thread X.
3) Thread X continues, remove the layer and... fire the layerRemoved event ! I let you guess what is going to happen here... AbstractMapPane is listening and... when the layers list change, it tries to reflect the changes in the display. So Thread X is now waiting for AbstractMapPane.drawingLock !
4) Here we are.. dead locked !
-------------
The solution ? Just remove the useless management of the 'monitor' lock in MapContent.xxx methods which access/modyfy the layers list.
-------------
For information...
I see two workarounds in that situation..
a) Thread X, when removing layers, does no longer calls MapContent.remove(myLayer)... but instead MapContent.layers().remove(myLayer).
This way, 'monitor' lock is just locked the time to return the layers list, BUT is no longer holded by Thread X while it does remove the layer from the list and notice the listeners.
b) This problem could also have been resolved in step 2, trying to access the MapContent.layers() BEFORE locking 'drawingLock'.
-------------
Hope I have well enough described the bug here, have a good day and happy coding !
|
