| Summary: | add code that protects against ConcurrentModificationExceptions to any places in the plugin container where we iterate a Resource's child Resources | ||
|---|---|---|---|
| Product: | [Other] RHQ Project | Reporter: | Ian Springer <ian.springer> |
| Component: | Plugin Container | Assignee: | Charles Crouch <ccrouch> |
| Status: | CLOSED CURRENTRELEASE | QA Contact: | Mike Foley <mfoley> |
| Severity: | medium | Docs Contact: | |
| Priority: | unspecified | ||
| Version: | 4.2 | CC: | ccrouch, hbrock, hrupp, mazz |
| Target Milestone: | --- | ||
| Target Release: | RHQ 4.3.0 | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Whiteboard: | |||
| Fixed In Version: | 4.3 | Doc Type: | Bug Fix |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2013-09-01 10:17:08 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
|
Description
Ian Springer
2012-02-17 16:37:01 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().
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.
Good idea. Done in master: http://git.fedorahosted.org/git/?p=rhq/rhq.git;a=commitdiff;h=6f7913a this fix caused this regression https://bugzilla.redhat.com/show_bug.cgi?id=797999 I think this issue is not fixed - see code analysis in bug #801432 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. |