Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WFCORE-4774] Avoid circular recursion searching for capability status #4038

Merged
merged 2 commits into from Dec 17, 2019
Merged

[WFCORE-4774] Avoid circular recursion searching for capability status #4038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2cbb255
[WFCORE-4774] Avoid circular recursion searching for the capability s…
yersan Dec 11, 2019
9041ff8
[WFCORE-4774] Add capability test cases when there are circular capab…
yersan Dec 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 11 additions & 10 deletions controller/src/main/java/org/jboss/as/controller/CapabilityRegistry.java
View file
Open in desktop
Expand Up @@ -352,14 +352,14 @@ public Map<CapabilityId, RuntimeStatus> getRuntimeStatus(PathAddress address, Im
if (size == 0) {
result = Collections.emptyMap();
} else {
Set<CapabilityId> examined = new HashSet<>();
Set<CapabilityId> visited = new HashSet<>();
if (size == 1) {
CapabilityId id = ids.iterator().next();
result = Collections.singletonMap(id, getCapabilityStatus(id, examined));
result = Collections.singletonMap(id, getCapabilityStatus(id, visited));
} else {
result = new HashMap<>(size);
for (CapabilityId id : ids) {
result.put(id, getCapabilityStatus(id, examined));
result.put(id, getCapabilityStatus(id, visited));
}
}
}
Expand All @@ -369,7 +369,7 @@ public Map<CapabilityId, RuntimeStatus> getRuntimeStatus(PathAddress address, Im
}
}

private RuntimeStatus getCapabilityStatus(CapabilityId id, Set<CapabilityId> examined) {
private RuntimeStatus getCapabilityStatus(CapabilityId id, Set<CapabilityId> visited) {
// This is meant for checking runtime stuff, which should only be for servers or
// HC runtime stuff, both of which use CapabilityScope.GLOBAL or HostCapabilityScope. So this assert
// is to check that assumption is valid, as further thought is needed if not (e.g. see WFCORE-1710).
Expand All @@ -385,11 +385,12 @@ private RuntimeStatus getCapabilityStatus(CapabilityId id, Set<CapabilityId> exa
if (reloadCapabilities.contains(id)) {
return RuntimeStatus.RELOAD_REQUIRED;
}
examined.add(id);
} // else defer reload-required check until after we search requirements for restart-required

visited.add(id);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only relevant change, move it outside of the previous if to make it count when we have capabilities in the restartCapabilities. I'm not sure what was the original intention, looks like it was expected to have it in the restartCapabilities array, but that's not always true. I renamed it to visited instead of examined, since examined could mean already checked its status, which is not correct at this point. At the end of this method, the capability is checked (examined) to see if it is in the reloadCapabilities. I found visited more adequate.


Map<String, RuntimeRequirementRegistration> dependents = requirements.get(id);
RuntimeStatus result = getDependentCapabilityStatus(dependents, id, examined);
RuntimeStatus result = getDependentCapabilityStatus(dependents, id, visited);
// TODO we could also check runtimeOnlyRequirements but it's not clear that's meaningful
// If the non-normal runtime-only req has had its cap removed, a RUNTIME step for the dependent
// will not see it any more and won't try and integrate. If the req is reload-required but
Expand All @@ -399,7 +400,7 @@ private RuntimeStatus getCapabilityStatus(CapabilityId id, Set<CapabilityId> exa
if (result != RuntimeStatus.RESTART_REQUIRED) {
// Check pending remove requirements
dependents = pendingRemoveRequirements.get(id);
RuntimeStatus pending = getDependentCapabilityStatus(dependents, id, examined);
RuntimeStatus pending = getDependentCapabilityStatus(dependents, id, visited);
if (pending != RuntimeStatus.NORMAL) {
result = pending;
}
Expand All @@ -412,7 +413,7 @@ private RuntimeStatus getCapabilityStatus(CapabilityId id, Set<CapabilityId> exa
return result;
}

private RuntimeStatus getDependentCapabilityStatus(Map<String, RuntimeRequirementRegistration> dependents, CapabilityId requiror, Set<CapabilityId> examined) {
private RuntimeStatus getDependentCapabilityStatus(Map<String, RuntimeRequirementRegistration> dependents, CapabilityId requiror, Set<CapabilityId> visited) {
RuntimeStatus result = RuntimeStatus.NORMAL;
if (dependents != null) {
for (String dependent : dependents.keySet()) {
Expand All @@ -422,8 +423,8 @@ private RuntimeStatus getDependentCapabilityStatus(Map<String, RuntimeRequiremen
: Arrays.asList(requirorScope, CapabilityScope.GLOBAL);
for (CapabilityScope scope : toCheck) {
CapabilityId dependentId = new CapabilityId(dependent, scope);
if (!examined.contains(dependentId)) {
RuntimeStatus status = getCapabilityStatus(dependentId, examined);
if (!visited.contains(dependentId)) {
RuntimeStatus status = getCapabilityStatus(dependentId, visited);
if (status == RuntimeStatus.RESTART_REQUIRED) {
return status; // no need to check anything else
} else if (status == RuntimeStatus.RELOAD_REQUIRED) {
Expand Down