per BZ traige (crouch, foley, loleary), medium priority, not targetted for JON 3.1

Actually triaging this into JON3.1 since it looks like https://bugzilla.redhat.com/show_bug.cgi?id=794790 didn't catch all the CMEs

(In reply to comment #2)
> Actually triaging this into JON3.1 since it looks like
> https://bugzilla.redhat.com/show_bug.cgi?id=794790 didn't catch all the CMEs

I don't think its that it didn't catch all CMEs, it looks like it put them back in.

For example, this issue shows the error happening in the stack:

Caused by: java.util.ConcurrentModificationException
...
        at
org.rhq.core.pc.inventory.InventoryManager.removeResourceAndIndicateIfScanIsNeeded(InventoryManager.java:1175)

Looking at commit 6f7913acb3583d89f8e1c5dd61f220cef1e05b9b which was for bug #794790, you will see this (notice it took OUT the copying of the child resources collection which was in here specifically to avoid the CME error):

we went from here (lines 1170-2):

            // put in new set to avoid concurrent mod exceptions
            Set<Resource> children = new HashSet<Resource>(resource.getChildResources());
            for (Resource child : children) {

to here (line 1176):

            for (Resource child : resource.getChildResources()) {

I see the same kind of thing in other places (that is, the commit removed the copying of the child collection - thus reintroducing the CME)... for example:

this (lines 1864-7):
                // Copy child Resources to an array and iterate that, rather than iterating the Set, which could cause
                // a ConcurrentModificationException if another thread tries to modify the Set while we're iterating it.
                Resource[] childResources = resource.getChildResources().toArray(new Resource[resource.getChildResources().size()]);
                for (Resource child : childResources) {

went to (line 1866):

                for (Resource child : resource.getChildResources()) {


Probably need Ian to take a look at his fix for 794790, because it looks like it caused regressions. I think we need to reopen bug #794790

Along with the removal of copying child collections, the fix also introduced the "Collections.newSetFromMap(new ConcurrentHashMap())" when a setting the child resources in InventoryManager#createNewResource().

That should remove the CMEs, too, but I guess that is not the only place we set the childresources collection.

A quick idea might be that the resources deserialized from the serverside wouldn't be treated with this code and thus the collection still be suspectible to the CMEs.

I make sure Resources from the server-side are cloned with a concurrent childResources field (find usages of InventoryManager.cloneResourceWithoutChildren()), but I must have missed some code path.

I'm here and I see a non-empty child resources set that is of type HashSet:

Daemon Thread [InventoryManager.discovery-1]
	Resource.getChildResources() line: 1375	
	InventoryManager.mergeResource(Resource) line: 2797	
	InventoryManager.mergeUnknownResources(Set<Integer>) line: 2687	
	InventoryManager.synchInventory(ResourceSyncInfo) line: 1089	
	InventoryManager.handleReport(InventoryReport) line: 1054	
	AutoDiscoveryExecutor.call() line: 131	

I was normally seeing the child resources set to be Collections$SetFromMap which I think is what we want now. So why is this still HashSet?

I cannot replicate this. however, I did push git commit in master3e93e33b82b96cc3cf73c54f5907ac8f9d97bbe6 only to ensure that our child collection remains as it was if it was already set. This really doesn't seem to fix anything but it at least serves to maintain our expectations.

I've run this through a debugger several times and the only time I see that the child collection is HashSet (as opposed to the thread-friendly Collections$SetFromMap) is from the stack trace I posted earlier (from mergeUnknownResources during a sync).

after staring at this for hours, I think I might see something:

from InventoryManager.removeResourceAndIndicateIfScanIsNeeded

            deactivateResource(resource);

            for (Resource child : resource.getChildResources()) {
                scanIsNeeded |= removeResourceAndIndicateIfScanIsNeeded(child);
            }

            Resource parent = resource.getParentResource();
            if (parent != null) {
                parent.removeChildResource(resource);
            }

Now, deactivateResource and, more importantly, removeResourceAndIndicateIfScanIsNeeded is recursive (as you see here).

What happens if one of the recursive calls hits the parent.removeChildResource(resource) line, returns and the for-loop tries to pick the next item out of the child set (for (Resource child : resource.getChildResources())) ?? That appears to be a potential source for ConcurrentModificationException? It is hard to see because you have to notice the recursive call.

I think we need to re-introduce the copying of the child set here. We'll have to re-examine all the places where the childResource set it used and see if there are any other recursive workflows that modify the set.

The ConcurrentHashMap allows simulatenous traversal and modification. So I do not think that the code snippet above should cause any problems iff the child resources collection is a set backed by this map:

- for loop creates an iterator over child resources set (set A)
  - recursive call (iterated value becomes the "resource")
    - for loop over children (set B)
    - parent.removeChildResource(resource) - this will remove the the resource that has just been iterated over in the set A
  - end of recursive call
  - iterator moves to the next element

so this is just iteration over and modification of a single set (or indeed the map that backs the set) in the same thread.

The following code simulates in a simple manner what is happening in the code snippet you identified:

Set<Object> objects = Collections.newSetFromMap(new ConcurrentHashMap<Object, Boolean>());
objects.add(new Object());
objects.add(new Object());
for(Object o : objects) objects.remove(o);

This code works OK. The CME *is* thrown though if the "objects" variable is initialized with a mere HashSet<Object> instead of the concurrent variant.

We need to identify the call chain that you once saw which leads to the resource child collection having type of HashSet and prevent that from happening.

Also note that, IMHO, the copy-before-iteration approach that we employed before didn't really work, but only lowered the chance of the CMEs happening.

If you look at the HashSet(Collection) constructor, you can see that it merely calls addAll(Collection) which *iterates* over the collection. So the CME still *would* happen if we for example removed a resource at the same time a scheduled discovery was running and child resources of the parent resource of the removed resource were being iterated over.

In short, we can't assume the inventory not changing out-of-band in any code that iterates over the resources because of the async nature of all of our operations on the inventory in the PC. 

There are several solutions to that - locking the subtree of a resource for every access to its children (i.e. essentially lock the whole inventory during a scheduled discovery for the whole time of the discovery (because this goes recursively top down from the platform level)), reimplement the discovery so that it is not a naive single-threaded recursive tree traversal - split it up in chunks that can happen concurrently, but you still need locking while traversing those chunks (currently we just hold N iterators "open" when doing discovery, where N is the depth of the resource tree) or employ concurrent-access-friendly collections.

> This code works OK. The CME *is* thrown though if the "objects" variable is
> initialized with a mere HashSet<Object> instead of the concurrent variant.

and this is what I am seeing - I was seeing a non-empty collection as a HashSet which causes the problem. I

IMHO, as I mentioned in that long post, the solution is not to re-introduce the copy-before-iteration concept, because I think it actually doesn't solve the CME problem in general, but to make sure that we are using the concurrent-safe collections in all workflows.

So looking at https://bugzilla.redhat.com/show_bug.cgi?id=801432#c6 is the issue finding out where this Resource instance came from? (since it couldn't have been created by InventoryManager since all the places its calls new Resource it passes in a ConcurrentHashMap based set for childresources, except when doing things like new Resource(resourceError.getResource().getId())))

If that is the issue, then can't you just temporarily add a member variable to Resource and in 

public Resource(Set<Resource> childResources)

set it to be Thread.currentThread().getStackTrace(). Then when you're in the debugger you can see where this rogue Resource is being created from.

I see server-side calls like this:

            Set<Resource> unknownResources = configuration.getServerServices().getDiscoveryServerService()
                .getResources(unknownResourceIds, true);

and through the debugger, I see coming back over the wire the child set is of type HashSet as expected. Now, the question is - do we squirrel away these resources that have child HashSet objects or do we operate on the hashset child object in a recursive way (while modifying those sets)? If so, we need to convert those sets to concurrent versions.

I am still not able to reproduce any ConcurrentModificationExceptions - I'm just going by the original BZ description and looking at the code and stepping through the debugger to find where it might occur. But I've yet to actually see the problem.

(In reply to comment #8)
> after staring at this for hours, I think I might see something:
> 
> from InventoryManager.removeResourceAndIndicateIfScanIsNeeded

I've examined this and I do not see any HashSet problems in here so far.

added test code to try to capture the stack trace when this happens, since I
can't replicate.

bug #807465 was written to remind us to delete that test code before release

All commits were made for RHQ4.4. all outstanding issues should have been addressed either in this bug or in related issues: 797999, 807465

Bulk closing of items that are on_qa and in old RHQ releases, which are out for a long time and where the issue has not been re-opened since.

OK, so in RHQ 4.12-ish I stumbled onto the log message added for this issue (see also Bug 807465).  In my agent log I saw messages like this when uninventorying resources:

2014-07-24 09:05:41,859 DEBUG [WorkerThread#0[127.0.0.1:54666]] (rhq.core.pc.inventory.InventoryManager)- [BZ 801432]
java.lang.Exception: Unexpected child set - if you see this, please notify support or log it in bugzillajava.util.HashSet:10230:test-simple.war
	at org.rhq.core.pc.inventory.InventoryManager.removeResourceAndIndicateIfScanIsNeeded(InventoryManager.java:1551)


This is a problem because it means that we are perhaps more susceptible to ConcurrentModificationException than we may have thought.  This indicates that we are working with HashSet when we are expecting to have the child resources in a safer Set impl.

After some investigation I found some issues, leading to this commit:

master commit 3e47741694b55fedecf9c904d89de11a7b92ccb2
Author: Jay Shaughnessy <jshaughn>
Date:   Thu Jul 24 22:22:55 2014 -0400

    Uninventory triggered our "assert-style" debug logging indicating that we
    were working with HashSet for child resources when we expected to be using
    a safer Set impl (wrt preventing ConcurrentModoficationException). There
    were a few issues in play:
    - When loading persisted resources from disk convert childResources to
      CopyOnWriteArraySet if necessary.  This repairs resources persisted with
      the wrong Set impl.
    - Eliminate some potential concurrent modification danger (or, at a minimum,
      some unnecessary work) by only calling deactivateResource() on the root
      resource. It recursively deactivates the subtree, so no need to then call
      it on every node in the subtree.
    - Fix a couple of other places where we weren't using CopyOnWriteArraySet
    - Fix Resource entity to ensure that when
      customChildResourcesCollection=true that the childResources Set is truly
      protected. It was here that we were losing the proper Set impl.
    - Fix our "assert-logging" conditional, which despite finding this issue was
      actually looking for the wrong set impl. It had not been updated when we
      moved to CopyOnWriteArraySet.