ConcurrentModificationException in DefaultCatalogFacade.getLayerByName

I just noticed a typo in the original test patch – this patch should replace it. I don't believe the change should have a significant affect on things.

BTW, I just noticed that the bugfix patch misses the layers re-initialization in resolve(). I will include this correction in a follow-up.

Thanks for the patch Eli. This all sounds good. I also see no reason why not to make all the lists, styles, layerGroups, etc... copyOnWrite lists as well. I'll post the updated patch.

@Andrea: what do you think?

Thanks Justin. I'm also wondering about the thread safety of the maps, which poses some more complexities. Specifically, ConcurrentHashMap does not support nulls (perhaps "" could be used instead) and the atomicity of MultiHashMap is questionable even if you override createCollection to use CopyOnWriteArrayList.

Indeed. I had this same though as well. I think ConcurrentHashMap with empty string keys + CopyOnWriteArrayList for the multi maps could work. Although I am bit cautious about that change this close to release as it is hard to say what the change of keys (null -> "") will lead to.

I wonder if it would be wise to apply this patch as is, and fix the map concurrency issue for 2.1.1 so it can get some intermediate testing before it goes out.

Thoughts?

I certainly agree with a more conservative approach on this, given the proximity to 2.1. I would be happy to see the layers change in 2.1. Overall, it seems like these conditions are fairly uncommon.

With respect to ConcurrentHashMap with empty string keys + CopyOnWriteArrayList, there is still a race condition when you have to create or remove the list entries which would require broader synchronization. Thinking longer term, could the nested maps be converted to simple concurrent structures? Is access through a key necessary? Could iteration with type checks suffice?

The short term approach of using CopyOnWriteArrayList for layers only seems fine, I'm just worried about servers where we have hundres of thousands of layers with hundreds of new layers being published daily, in that case the copy could become an expensive one (just recently seen a server with 160.000 layers, for that one publishing a new layer would result in the copy of 640KB on a 64bit VM).
A tempting alternative could be to wrap the list with a custom wrapper using a ReadWriteLock, however iterators do not have any close method so there is no way to tell for sure when an iteration has been completed...

Longer term we might want to consider using ConcurrentHashSet instead of the lists... I don't think we have any requirement to keep the layers in any particular order, don't we?

Good points. Ok, for now lets keep it to the original patch and CopyOnArrayList for layers only.

ConcurrentHashSet sounds like the best idea... except for that it involves changing the catalog api. I can't think of reason to maintain order off the top of my head but I would be surprised if something did not break due to the change.

I'll open a separate issue to generally improve concurrency in DefaultCatalogFacade moving it to use ConcurrentHashMap and ConcurrentHashSet... and hopefully get to it for 2.1.1. Agreed?

@Andrea - That is a very good point. Your suggestion of a ReadWriteLock seems viable at least for the DefaultCatalogFacade List fields, as their references don't seem to ever escape DefaultCatalogFacade. The attached patch resolves the issue through the use of ReadWriteLock instead of CopyOnWriteArrayList.

Thanks for the quick patch Eli... but I am thinking for now I would push for just using the CopyOnWriteArrayList as the temporary measure and for 2.1.1 move to using concurrent collections to handle this for us, rather than writing a lot of locking code on our own. A lot of the lock management here is duplicated for every layer access... multiply that by the same code for access of every entity in the catalog it becomes a lot to maintain. So if we can get away with just using concurrent collections it would be a lot cleaner and less error prone.

@Justin - That's no problem. I absolutely agree with the approach. When it comes time to make more of these changes I would be happy to help out.

Justin, I'm worried that just using the copy on write approach will render GS almost useless for some of the GeoServer support contracts I attend to (those that have a ton of layers).
If it's ok I'd prefer to review the patch using the read write lock, with the agreement that it's just a temporary measure, to be removed when we switch to concurrent sets.
If it's not ok I I'll just have to tell some people to stay away from GS 2.1 final (that would not be a major problem if we switch to concurrent collections right after
the 2.1.0 release, or I can just make a short lived fork for them)

Andrea, I understand the concern. The reason I pushed for the copy on write approach (in the short term) was that it seemed less risky. The rwl approach seems far from trivial so I am cautious about introducing it this close to 2.1 without any testing first. Also since there are methods that already copy the entire list of layers (like getLayers()) it did not seem so outrageous.

That said I can't see any faults with rwl patch for the most part... except for the following chunk:

         if ( layers == null ) {
-            layers = new ArrayList<LayerInfo>();    
+            layersWrite.lock();
+            try {
+                layers = new ArrayList<LayerInfo>();    
+            } finally {
+                layersWrite.unlock();
+            }
         }
-        for ( LayerInfo l : layers ) { 
-            resolve(l);
-        }
+        // this appears to be dead code
+        //for ( LayerInfo l : layers ) { 
+        //    resolve(l);
+        //}

Eli: Can you explain why the loop through the layers to call resolve is removed?

Justin, you're probably right I'm worrying too much, indeed getLayers() is used in a lot of places and it actually makes a defensive copy (I though it was returning a read only list wrapper, but it's not).
Let's go ahead with the CopyOnWriteArrayList, if that really causes troubles with those few deploys I'll just have them sit on 2.1RC2 until the problem is solved by using the concurrent sets

@Justin - I had commented out the layers loop because it appeared that it would always be a no-op because layers had just been initialized to an empty list (assuming that there were no concurrent side effects).

Committed the workaround for now and created GEOS-4413 for long term solution.

Mass closing all issues that have been in "resolved" state for more than a month without further comments