From 1b23794446701765370749e7a6430df98a72a619 Mon Sep 17 00:00:00 2001 From: Martin Pokorny <89339813+mPokornyETM@users.noreply.github.com> Date: Thu, 11 Jan 2024 17:45:59 +0100 Subject: [PATCH] Eliminate ConcurrentModificationException in free style projects (#604) --- .../LockableResourcesManager.java | 5 + .../queue/LockRunListener.java | 108 ++++++++++-------- 2 files changed, 67 insertions(+), 46 deletions(-) diff --git a/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java b/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java index b12d53b21..310c2fd13 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java @@ -533,6 +533,11 @@ public synchronized boolean lock(List resources, Run bui return lock(resources, build, context, null, null, false); } + @Restricted(NoExternalUse.class) + public synchronized boolean lock(List resources, Run build) { + return lock(resources, build, null); + } + /** Try to lock the resource and return true if locked. */ public synchronized boolean lock( List resources, diff --git a/src/main/java/org/jenkins/plugins/lockableresources/queue/LockRunListener.java b/src/main/java/org/jenkins/plugins/lockableresources/queue/LockRunListener.java index db0f91f26..8d0063625 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/queue/LockRunListener.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/queue/LockRunListener.java @@ -40,48 +40,53 @@ public void onStarted(Run build, TaskListener listener) { if (build instanceof MatrixBuild) return; if (build instanceof AbstractBuild) { - Job proj = Utils.getProject(build); - List required = new ArrayList<>(); - LockableResourcesStruct resources = Utils.requiredResources(proj); + LockableResourcesManager lrm = LockableResourcesManager.get(); + synchronized (lrm.syncResources) { + Job proj = Utils.getProject(build); + List required = new ArrayList<>(); + LockableResourcesStruct resources = Utils.requiredResources(proj); - if (resources != null) { - if (resources.requiredNumber != null - || !resources.label.isEmpty() - || resources.getResourceMatchScript() != null) { - required.addAll(LockableResourcesManager.get().getResourcesFromProject(proj.getFullName())); - } else { - required.addAll(resources.required); - } + if (resources != null) { + if (resources.requiredNumber != null + || !resources.label.isEmpty() + || resources.getResourceMatchScript() != null) { + required.addAll(lrm.getResourcesFromProject(proj.getFullName())); + } else { + required.addAll(resources.required); + } - if (LockableResourcesManager.get().lock(required, build, null)) { - build.addAction(LockedResourcesBuildAction.fromResources(required)); - listener.getLogger().printf("%s acquired lock on %s%n", LOG_PREFIX, required); - LOGGER.fine(build.getFullDisplayName() + " acquired lock on " + required); - if (resources.requiredVar != null) { - List envsToSet = new ArrayList<>(); + if (lrm.lock(required, build)) { + build.addAction(LockedResourcesBuildAction.fromResources(required)); + listener.getLogger().printf("%s acquired lock on %s%n", LOG_PREFIX, required); + LOGGER.info(build.getFullDisplayName() + " acquired lock on " + required); + if (resources.requiredVar != null) { + List envsToSet = new ArrayList<>(); - // add the comma separated list of names acquired - envsToSet.add(new StringParameterValue( - resources.requiredVar, - required.stream().map(LockableResource::getName).collect(Collectors.joining(",")))); + // add the comma separated list of names acquired + envsToSet.add(new StringParameterValue( + resources.requiredVar, + required.stream() + .map(LockableResource::getName) + .collect(Collectors.joining(",")))); - // also add a numbered variable for each acquired lock along with properties of the lock - int index = 0; - for (LockableResource lr : required) { - String lockEnvName = resources.requiredVar + index; - envsToSet.add(new StringParameterValue(lockEnvName, lr.getName())); - for (LockableResourceProperty lockProperty : lr.getProperties()) { - String propEnvName = lockEnvName + "_" + lockProperty.getName(); - envsToSet.add(new StringParameterValue(propEnvName, lockProperty.getValue())); + // also add a numbered variable for each acquired lock along with properties of the lock + int index = 0; + for (LockableResource lr : required) { + String lockEnvName = resources.requiredVar + index; + envsToSet.add(new StringParameterValue(lockEnvName, lr.getName())); + for (LockableResourceProperty lockProperty : lr.getProperties()) { + String propEnvName = lockEnvName + "_" + lockProperty.getName(); + envsToSet.add(new StringParameterValue(propEnvName, lockProperty.getValue())); + } + ++index; } - ++index; - } - build.addAction(new ResourceVariableNameAction(envsToSet)); + build.addAction(new ResourceVariableNameAction(envsToSet)); + } + } else { + listener.getLogger().printf("%s failed to lock %s%n", LOG_PREFIX, required); + LOGGER.warning(build.getFullDisplayName() + " failed to lock " + required); } - } else { - listener.getLogger().printf("%s failed to lock %s%n", LOG_PREFIX, required); - LOGGER.fine(build.getFullDisplayName() + " failed to lock " + required); } } } @@ -93,12 +98,18 @@ public void onCompleted(Run build, @NonNull TaskListener listener) { // only the child jobs will actually unlock resources. if (build instanceof MatrixBuild) return; - // obviously project name cannot be obtained here - List required = LockableResourcesManager.get().getResourcesFromBuild(build); - if (!required.isEmpty()) { - LockableResourcesManager.get().unlock(required, build); - listener.getLogger().printf("%s released lock on %s%n", LOG_PREFIX, required); - LOGGER.fine(build.getFullDisplayName() + " released lock on " + required); + LockableResourcesManager lrm = LockableResourcesManager.get(); + synchronized (lrm.syncResources) { + // obviously project name cannot be obtained here + List required = lrm.getResourcesFromBuild(build); + if (!required.isEmpty()) { + lrm.unlock(required, build); + listener.getLogger().printf("%s released lock on %s%n", LOG_PREFIX, required); + LOGGER.info(build.getFullDisplayName() + + " released lock on " + + required + + ", because the build has been finished."); + } } } @@ -107,11 +118,16 @@ public void onDeleted(Run build) { // Skip unlocking for multiple configuration projects, // only the child jobs will actually unlock resources. if (build instanceof MatrixBuild) return; - - List required = LockableResourcesManager.get().getResourcesFromBuild(build); - if (!required.isEmpty()) { - LockableResourcesManager.get().unlock(required, build); - LOGGER.fine(build.getFullDisplayName() + " released lock on " + required); + LockableResourcesManager lrm = LockableResourcesManager.get(); + synchronized (lrm.syncResources) { + List required = lrm.getResourcesFromBuild(build); + if (!required.isEmpty()) { + lrm.unlock(required, build); + LOGGER.warning(build.getFullDisplayName() + + " released lock on " + + required + + ", because the build has been deleted."); + } } } }