[801432, 807465] Related Issue to these BZs

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,
- 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.
  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 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.

modules/core/domain/src/main/java/org/rhq/core/domain/resource/Resource.java

@@ -247,7 +247,8 @@
     @NamedQuery(name = Resource.QUERY_FIND_RESOURCE_AUTOGROUP_COMPOSITE_ADMIN, query = "" //
         + "  SELECT new org.rhq.core.domain.resource.group.composite.AutoGroupComposite(avg(a.availabilityType), res.parentResource, rt, count(res)) "
         + "    FROM Resource res JOIN res.currentAvailability a JOIN res.resourceType rt "
-        + "   WHERE res.id = :id " + "GROUP BY res.parentResource, rt"),
+        + "   WHERE res.id = :id "
+        + "GROUP BY res.parentResource, rt"),
     @NamedQuery(name = Resource.QUERY_FIND_RESOURCE_AUTOGROUPS_COMPOSITE_ADMIN, query = "" //
         + "  SELECT new org.rhq.core.domain.resource.group.composite.AutoGroupComposite(avg(a.availabilityType), res.parentResource, rt, count(res)) "
         + "    FROM Resource res JOIN res.currentAvailability a JOIN res.resourceType rt "
@@ -1455,17 +1456,16 @@ public Set<Resource> getChildResources() {
 
     public void addChildResource(Resource childResource) {
         childResource.setParentResource(this);
-        if (null == this.childResources || this.childResources.equals(Collections.emptySet())) {
+        if (null == this.childResources
+            || (!customChildResourcesCollection && this.childResources.equals(Collections.emptySet()))) {
             this.childResources = new HashSet<Resource>(1);
         }
         this.childResources.add(childResource);
     }
 
     public void addChildResourceWithoutAncestry(Resource childResource) {
         childResource.setParentResourceWithoutAncestry(this);
-        if (null == this.childResources || childResources.equals(Collections.emptySet())) {
-            this.childResources = new HashSet<Resource>(1);
-        }
+        addChildResource(childResource);
         this.childResources.add(childResource);
     }
 
@@ -1478,7 +1478,12 @@ public boolean removeChildResource(Resource childResource) {
     }
 
     public void setChildResources(Set<Resource> children) {
-        this.childResources = children;
+        if (customChildResourcesCollection) {
+            this.childResources.clear();
+            this.childResources.addAll(children);
+        } else {
+            this.childResources = children;
+        }
     }
 
     public Resource getParentResource() {

modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/InventoryManager.java

@@ -1516,7 +1516,7 @@ public void uninventoryResource(int resourceId) {
             }
             return;
         }
-        boolean scan = removeResourceAndIndicateIfScanIsNeeded(resourceContainer.getResource());
+        boolean scan = removeResourceAndIndicateIfScanIsNeeded(resourceContainer.getResource(), true);
 
         //only actually schedule the scanning when we are finished with resource upgrade. The resource upgrade
         //happens before any scanning infrastructure is established.
@@ -1532,7 +1532,7 @@ public void uninventoryResource(int resourceId) {
      * @param resource the Resource to be removed
      * @return true if this method deleted things that requires a scan.
      */
-    boolean removeResourceAndIndicateIfScanIsNeeded(Resource resource) {
+    boolean removeResourceAndIndicateIfScanIsNeeded(Resource resource, boolean isRoot) {
         boolean scanIsNeeded = false;
 
         this.inventoryLock.writeLock().lock();
@@ -1541,25 +1541,27 @@ boolean removeResourceAndIndicateIfScanIsNeeded(Resource resource) {
                 log.debug("Removing [" + resource + "] from local inventory...");
             }
 
-            // this will deactivate the resource starting bottom-up - so this ends up as a no-op if we are being called
-            // recursively, but we need to do this now to ensure everything is stopped prior to removing them from inventory
-            deactivateResource(resource);
+            // deactivateResource recursively deactivates the resource and its children. No need to do this
+            // if we are recursing as well.
+            if (isRoot) {
+                deactivateResource(resource);
+            }
 
+            Set<Resource> children = getContainerChildren(resource);
             // see BZ 801432
             if (log.isDebugEnabled()) {
-                if (!resource.getChildResources().getClass().getName().contains("Collections$SetFromMap")) {
+                if ((children.size() > 0) && (!(children instanceof CopyOnWriteArraySet))) {
                     Exception e = new Exception(
-                        "Unexpected child set - if you see this, please notify support or log it in bugzilla"
-                            + resource.getChildResources().getClass().getName() + ":" + resource.getId() + ":"
-                            + resource.getName());
+                        "Unexpected child set - if you see this, please notify support or log it in bugzilla. "
+                            + children.getClass().getName() + ":" + resource.getId() + ":" + resource.getName()
+                            + ":numChildResources=" + children.size());
                     log.debug("[BZ 801432]", e);
                 }
             }
 
-            Set<Resource> children = getContainerChildren(resource);
             Set<Resource> tmp = new HashSet<Resource>(children);
             for (Resource child : tmp) {
-                scanIsNeeded |= removeResourceAndIndicateIfScanIsNeeded(child);
+                scanIsNeeded |= removeResourceAndIndicateIfScanIsNeeded(child, false);
             }
 
             Resource parent = resource.getParentResource();
@@ -2434,12 +2436,14 @@ private void loadFromDisk() {
                 inventoryFile.loadInventory();
 
                 this.platform = inventoryFile.getPlatform();
+                practiceSafeSets(this.platform);
                 this.resourceContainersByUUID.clear();
                 this.resourceContainerByResourceId.clear();
                 for (String uuid : inventoryFile.getResourceContainers().keySet()) {
                     ResourceContainer resourceContainer = inventoryFile.getResourceContainers().get(uuid);
                     this.resourceContainersByUUID.put(uuid, resourceContainer);
                     Resource resource = resourceContainer.getResource();
+                    practiceSafeSets(resource);
                     this.resourceContainerByResourceId.put(resource.getId(), resourceContainer);
                     compactResource(resource);
                 }
@@ -2462,6 +2466,20 @@ private void loadFromDisk() {
         }
     }
 
+    // Make sure the child resources are in our desired Set impl
+    private void practiceSafeSets(final Resource resource) {
+        Set<Resource> children = resource.getChildResources();
+        if (null == children) {
+            resource.setChildResources(new CopyOnWriteArraySet<Resource>());
+        } else if (!(children instanceof CopyOnWriteArraySet)) {
+            if (log.isDebugEnabled()) {
+                log.debug("Converting persisted childResources to CopyOnWriteArraySet from ["
+                    + children.getClass().getSimpleName() + "] for [" + resource.getName() + "]");
+            }
+            resource.setChildResources(new CopyOnWriteArraySet<Resource>(children));
+        }
+    }
+
     /**
      * Shutdown the ResourceComponents from the bottom up.
      * @param resource The resource to deactivate
@@ -2619,7 +2637,8 @@ private Resource getTestPlatform() {
         if (this.platform != null && this.platform.getResourceType() == type) {
             return this.platform;
         }
-        Set<Resource> childResources = Collections.newSetFromMap(new ConcurrentHashMap<Resource, Boolean>());
+        Set<Resource> childResources = new CopyOnWriteArraySet<Resource>(
+            Collections.newSetFromMap(new ConcurrentHashMap<Resource, Boolean>()));
         Resource platform = new Resource(childResources);
         platform.setResourceKey("testkey" + configuration.getContainerName());
         platform.setName("testplatform");
@@ -3348,7 +3367,7 @@ private Set<Resource> getResourcesFromSyncInfos(Set<ResourceSyncInfo> syncInfos)
             for (Resource r : resourceBatch) {
                 //  protect against childResources notNull assumptions downstream
                 if (null == r.getChildResources()) {
-                    r.setChildResources(Collections.EMPTY_SET); // this will actually initialize to an empty Set
+                    r.setChildResources(new CopyOnWriteArraySet()); // this will actually initialize to an empty Set
                 }
                 compactResource(r);
                 resourceMap.put(r.getId(), r);

modules/core/plugin-container/src/main/java/org/rhq/core/pc/inventory/RuntimeDiscoveryExecutor.java

@@ -325,7 +325,7 @@ private void removeStaleResources(Resource parent, ResourceType childResourceTyp
                 && !mergedResources.containsKey(existingChildResource.getUuid())
                 && (existingChildResource.getId() == 0 || !this.pluginContainerConfiguration.isInsideAgent())) {
                 log.info("Removing stale resource [" + existingChildResource + "]");
-                this.inventoryManager.removeResourceAndIndicateIfScanIsNeeded(existingChildResource);
+                this.inventoryManager.removeResourceAndIndicateIfScanIsNeeded(existingChildResource, true);
             }
         }
     }