Skip to content

Commit

Permalink
Eliminate ConcurrentModificationException on plugin load (Jenkins sta…
Browse files Browse the repository at this point in the history
…rtup) (#586)
  • Loading branch information
mPokornyETM authored Dec 6, 2023
1 parent 4e62e7f commit a3dd45e
Show file tree
Hide file tree
Showing 6 changed files with 290 additions and 87 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,29 @@ private BackwardCompatibility() {}

@Initializer(after = InitMilestone.JOB_LOADED)
public static void compatibilityMigration() {
List<LockableResource> resources = LockableResourcesManager.get().getResources();
LOG.log(
Level.FINE,
"lockable-resources-plugin compatibility migration task run for " + resources.size() + " resources");
for (LockableResource resource : resources) {
List<StepContext> queuedContexts = resource.getQueuedContexts();
if (!queuedContexts.isEmpty()) {
for (StepContext queuedContext : queuedContexts) {
List<String> resourcesNames = new ArrayList<>();
resourcesNames.add(resource.getName());
LockableResourcesStruct resourceHolder = new LockableResourcesStruct(resourcesNames, "", 0);
LockableResourcesManager.get()
.queueContext(
queuedContext, Collections.singletonList(resourceHolder), resource.getName(), null);
LockableResourcesManager lrm = LockableResourcesManager.get();
synchronized (lrm.syncResources) {
List<LockableResource> resources = lrm.getResources();
LOG.log(
Level.FINE,
"lockable-resources-plugin compatibility migration task run for " + resources.size()
+ " resources");
for (LockableResource resource : resources) {
List<StepContext> queuedContexts = resource.getQueuedContexts();
if (!queuedContexts.isEmpty()) {
for (StepContext queuedContext : queuedContexts) {
List<String> resourcesNames = new ArrayList<>();
resourcesNames.add(resource.getName());
LockableResourcesStruct resourceHolder = new LockableResourcesStruct(resourcesNames, "", 0);
LockableResourcesManager.get()
.queueContext(
queuedContext,
Collections.singletonList(resourceHolder),
resource.getName(),
null);
}
queuedContexts.clear();
}
queuedContexts.clear();
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ private FreeDeadJobs() {}
public static void freePostMortemResources() {

LockableResourcesManager lrm = LockableResourcesManager.get();
synchronized (lrm) {
synchronized (lrm.syncResources) {
LOG.log(Level.FINE, "lockable-resources-plugin free post mortem task run");
for (LockableResource resource : lrm.getResources()) {
if (resource.getBuild() != null && !resource.getBuild().isInProgress()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import com.infradna.tool.bridge_method_injector.WithBridgeMethods;
import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.Nullable;
import groovy.lang.Binding;
import hudson.Extension;
import hudson.Util;
Expand Down Expand Up @@ -130,7 +131,7 @@ public LockableResource(String name, String description, String labels, String r
}

@DataBoundConstructor
public LockableResource(String name) {
public LockableResource(@CheckForNull String name) {
this.name = Util.fixNull(name);
// todo throw exception, when the name is empty
// todo check if the name contains only valid characters (no spaces, new lines ...)
Expand Down Expand Up @@ -185,7 +186,7 @@ public String getDescription() {
}

@DataBoundSetter
public void setDescription(String description) {
public void setDescription(@Nullable String description) {
this.description = Util.fixNull(description);
}

Expand All @@ -195,7 +196,7 @@ public String getNote() {
}

@DataBoundSetter
public void setNote(String note) {
public void setNote(@Nullable String note) {
this.note = Util.fixNull(note);
}

Expand Down Expand Up @@ -230,7 +231,8 @@ public String getLabels() {
*/
// @Deprecated can not be used, because of JCaC
@DataBoundSetter
public void setLabels(String labels) {
public void setLabels(@Nullable String labels) {

Check warning on line 234 in src/main/java/org/jenkins/plugins/lockableresources/LockableResource.java

View check run for this annotation

ci.jenkins.io / Java Compiler

compiler:compile

NORMAL: deprecated item is not annotated with @deprecated
labels = Util.fixNull(labels);
// todo use label parser from Jenkins.Label to allow the same syntax
this.labelsAsList = new ArrayList<>();
for (String label : labels.split("\\s+")) {
Expand Down Expand Up @@ -258,7 +260,7 @@ public List<String> getLabelsAsList() {
* @return {@code true} if this resource contains the label.
*/
@Exported
public boolean hasLabel(String labelToFind) {
public boolean hasLabel(@CheckForNull String labelToFind) {
return this.labelsContain(labelToFind);
}

Expand All @@ -278,8 +280,9 @@ public boolean isValidLabel(String candidate, Map<String, Object> params) {
* https://www.jenkins.io/doc/pipeline/steps/workflow-durable-task-step/#node-allocate-node).
* Valid means that the resource contains the label or the Label-expression matched.
*/
public boolean isValidLabel(String candidate) {
if (candidate == null || candidate.isEmpty()) {
public boolean isValidLabel(@Nullable String candidate) {
candidate = Util.fixEmptyAndTrim(Util.fixNull(candidate));
if (candidate == null) {

Check warning on line 285 in src/main/java/org/jenkins/plugins/lockableresources/LockableResource.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 285 is only partially covered, one branch is missing
return false;
}

Expand Down Expand Up @@ -313,7 +316,7 @@ public List<LockableResourceProperty> getProperties() {
}

@DataBoundSetter
public void setProperties(List<LockableResourceProperty> properties) {
public void setProperties(@Nullable List<LockableResourceProperty> properties) {
this.properties = (properties == null ? new ArrayList<>() : properties);
}

Expand Down Expand Up @@ -359,7 +362,7 @@ public Date getReservedTimestamp() {
}

@DataBoundSetter
public void setReservedTimestamp(final Date reservedTimestamp) {
public void setReservedTimestamp(@Nullable final Date reservedTimestamp) {
this.reservedTimestamp = reservedTimestamp == null ? null : new Date(reservedTimestamp.getTime());
}

Expand Down Expand Up @@ -495,7 +498,7 @@ public String getBuildName() {
else return null;
}

public void setBuild(Run<?, ?> lockedBy) {
public void setBuild(@Nullable Run<?, ?> lockedBy) {
this.build = lockedBy;
if (lockedBy != null) {
this.buildExternalizableId = lockedBy.getExternalizableId();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@
@Extension
public class LockableResourcesManager extends GlobalConfiguration {

/** Object to synchronized operations over LRM */
public static final transient Object syncResources = new Object();

private List<LockableResource> resources;
private transient Cache<Long, List<LockableResource>> cachedCandidates =
CacheBuilder.newBuilder().expireAfterWrite(5, TimeUnit.MINUTES).build();
Expand Down Expand Up @@ -221,7 +224,9 @@ public List<LockableResource> getResourcesWithLabel(String label, Map<String, Ob
@NonNull
@Restricted(NoExternalUse.class)
public List<LockableResource> getResourcesWithLabel(final String label) {
return _getResourcesWithLabel(label, this.getResources());
synchronized (this.syncResources) {
return _getResourcesWithLabel(label, this.getResources());
}
}

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -270,6 +275,15 @@ public synchronized LockableResource fromName(String resourceName) {
return null;
}

// ---------------------------------------------------------------------------
private String getStack() {
StringBuffer buf = new StringBuffer();
for (StackTraceElement st : Thread.currentThread().getStackTrace()) {
buf.append("\n" + st);
}
return buf.toString();
}

// ---------------------------------------------------------------------------
/** Checks if given resource exist. */
@NonNull
Expand Down Expand Up @@ -729,59 +743,74 @@ public List<QueuedContextStruct> getCurrentQueuedContext() {
return Collections.unmodifiableList(this.queuedContexts);
}

// ---------------------------------------------------------------------------
/** Creates the resource if it does not exist. */
public synchronized boolean createResource(String name) {
public boolean createResource(@CheckForNull String name) {
name = Util.fixEmptyAndTrim(name);
if (name != null) {
LockableResource existent = fromName(name);
if (existent == null) {
LockableResource resource = new LockableResource(name);
resource.setEphemeral(true);
getResources().add(resource);
save();
return true;
}
}
return false;
LockableResource resource = new LockableResource(name);
resource.setEphemeral(true);

return this.addResource(resource, /*doSave*/ true);
}

public synchronized boolean createResourceWithLabel(String name, String label) {
// ---------------------------------------------------------------------------
public boolean createResourceWithLabel(@CheckForNull String name, @CheckForNull String label) {
name = Util.fixEmptyAndTrim(name);
if (name != null && label != null) {
LockableResource existent = fromName(name);
if (existent == null) {
LockableResource resource = new LockableResource(name);
resource.setLabels(label);
getResources().add(resource);
save();
return true;
}
label = Util.fixEmptyAndTrim(label);
LockableResource resource = new LockableResource(name);
resource.setLabels(label);

return this.addResource(resource, /*doSave*/ true);
}

// ---------------------------------------------------------------------------
public boolean createResourceWithLabelAndProperties(
@CheckForNull String name, @CheckForNull String label, final Map<String, String> properties) {
if (properties == null) {

Check warning on line 769 in src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 769 is only partially covered, one branch is missing
return false;
}
return false;

name = Util.fixEmptyAndTrim(name);
label = Util.fixEmptyAndTrim(label);
LockableResource resource = new LockableResource(name);
resource.setLabels(label);
resource.setProperties(properties.entrySet().stream()
.map(e -> {
LockableResourceProperty p = new LockableResourceProperty();
p.setName(e.getKey());
p.setValue(e.getValue());
return p;
})
.collect(Collectors.toList()));

return this.addResource(resource, /*doSave*/ true);
}

// ---------------------------------------------------------------------------
@Restricted(NoExternalUse.class)
public synchronized boolean createResourceWithLabelAndProperties(
String name, String label, Map<String, String> properties) {
if (name != null && label != null && properties != null) {
LockableResource existent = fromName(name);
if (existent == null) {
LockableResource resource = new LockableResource(name);
resource.setLabels(label);
resource.setProperties(properties.entrySet().stream()
.map(e -> {
LockableResourceProperty p = new LockableResourceProperty();
p.setName(e.getKey());
p.setValue(e.getValue());
return p;
})
.collect(Collectors.toList()));
getResources().add(resource);
save();
return true;
public boolean addResource(@Nullable final LockableResource resource) {
return this.addResource(resource, /*doSave*/ false);
}
// ---------------------------------------------------------------------------
@Restricted(NoExternalUse.class)
public boolean addResource(@Nullable final LockableResource resource, final boolean doSave) {

if (resource == null || resource.getName() == null || resource.getName().isEmpty()) {

Check warning on line 798 in src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 798 is only partially covered, 3 branches are missing
LOGGER.warning("Internal failure: We will add wrong resource: " + resource + getStack());

Check warning on line 799 in src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 799 is not covered by tests
return false;
}
synchronized (this.syncResources) {
if (this.resourceExist(resource.getName())) {
LOGGER.finest("We will add existing resource: " + resource + getStack());
return false;
}
this.resources.add(resource);
LOGGER.fine("Resource added : " + resource);
if (doSave) {
this.save();
}
}
return false;
return true;
}

/**
Expand Down
Loading

0 comments on commit a3dd45e

Please sign in to comment.