Bug 794790 - add code that protects against ConcurrentModificationExceptions to any places in the plugin container where we iterate a Resource's child Resources
Summary: add code that protects against ConcurrentModificationExceptions to any places...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: RHQ Project
Classification: Other
Component: Plugin Container
Version: 4.2
Hardware: Unspecified
OS: Unspecified
unspecified
medium
Target Milestone: ---
: RHQ 4.3.0
Assignee: Charles Crouch
QA Contact: Mike Foley
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-02-17 16:37 UTC by Ian Springer
Modified: 2015-02-01 23:27 UTC (History)
4 users (show)

Fixed In Version: 4.3
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-09-01 10:17:08 UTC
Embargoed:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Bugzilla 801432 0 high CLOSED ConcurrentModificationException on agent when resource is ignored in the discovery queue 2021-02-22 00:41:40 UTC

Internal Links: 801432

Description Ian Springer 2012-02-17 16:37:01 UTC

Comment 1 Ian Springer 2012-02-17 16:40:22 UTC
Here's an example of how to guard against

  // 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 childResource : childResources) {
     // do something w/ child Resource
  }

To find all the places that may need fixing, do a find usages of Resource.getChildResources().

Comment 2 Charles Crouch 2012-02-17 17:33:20 UTC
Are we sure that copying the set into an array is the best solution?
Have we thought about doing something like this in org.rhq.core.domain.resource.Resource:


private Set<Resource> childResources = Collections.newSetFromMap(
     new ConcurrentHashMap<Resource, Boolean>());

versus what we have now:

private Set<Resource> childResources = new LinkedHashSet<Resource>();

How critical is the *Linked* part of LinkedHastSet, i.e. that we need to iterate over the set in some defined order (insertion-order)? If its critical then maybe we could look at:

http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/CopyOnWriteArraySet.html

Noting: "[CopyOnWriteArraySet] is best suited for applications in which set sizes generally stay small, read-only operations vastly outnumber mutative operations, and you need to prevent interference among threads during traversal."

It looks like this would have to change too:
  public void setChildResources(Set<Resource> children) {
         if (children == null) {
             children = new LinkedHashSet<Resource>();
         }
         this.childResources = children;
     }

Given we currently allow people to supply their own Set implementation through setChildResources I presume we can't be relying on the fact we'll always have a LinkedHashSet.

Comment 3 Ian Springer 2012-02-20 18:08:46 UTC
Good idea. 

Done in master: http://git.fedorahosted.org/git/?p=rhq/rhq.git;a=commitdiff;h=6f7913a

Comment 4 Mike Foley 2012-02-29 23:22:41 UTC
this fix caused this regression 

https://bugzilla.redhat.com/show_bug.cgi?id=797999

Comment 5 John Mazzitelli 2012-03-26 15:54:33 UTC
I think this issue is not fixed - see code analysis in bug #801432

Comment 8 Heiko W. Rupp 2013-09-01 10:17:08 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.