From ba22ac42b44b004eed0c2ed6d89daef080ea381f Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 24 Oct 2025 11:29:03 -0400 Subject: [PATCH] Introduce `ThrowingRunnable` and `ThrowingCallable` (#11037) * Introduce `ThrowingRunnable` and `ThrowingCallable` * Minimize diff * Minimize diff * Deprecate `NotReallyRoleSensitiveCallable` https://github.com/jenkinsci/jenkins/pull/11037#discussion_r2323626187 * CheckStyle --------- Co-authored-by: Mark Waite --- .../main/java/hudson/model/AbstractItem.java | 18 +---- core/src/main/java/hudson/model/Executor.java | 60 +++++++------- .../java/hudson/model/ItemGroupMixIn.java | 13 +-- core/src/main/java/hudson/model/Items.java | 35 +++++++- core/src/main/java/hudson/model/Queue.java | 81 +++++++++++++++++-- .../java/hudson/model/ResourceController.java | 69 +++++++--------- core/src/main/java/hudson/security/ACL.java | 7 +- .../hudson/slaves/ComputerRetentionWork.java | 19 ++--- .../java/hudson/slaves/RetentionStrategy.java | 2 +- .../SimpleScheduledRetentionStrategy.java | 30 +++---- .../java/hudson/slaves/SlaveComputer.java | 10 +-- core/src/main/java/jenkins/model/Nodes.java | 70 +++++++--------- .../NotReallyRoleSensitiveCallable.java | 8 +- .../java/jenkins/util/ThrowingCallable.java | 54 +++++++++++++ .../java/jenkins/util/ThrowingRunnable.java | 42 ++++++++++ 15 files changed, 323 insertions(+), 195 deletions(-) create mode 100644 core/src/main/java/jenkins/util/ThrowingCallable.java create mode 100644 core/src/main/java/jenkins/util/ThrowingRunnable.java diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java index 38900ddb61..8c25615660 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -69,7 +69,6 @@ import jenkins.model.Jenkins; import jenkins.model.Loadable; import jenkins.model.queue.ItemDeletion; import jenkins.security.ExtendedReadRedaction; -import jenkins.security.NotReallyRoleSensitiveCallable; import jenkins.security.stapler.StaplerNotDispatchable; import jenkins.util.SystemProperties; import jenkins.util.xml.XMLUtils; @@ -914,6 +913,7 @@ public abstract class AbstractItem extends Actionable implements Loadable, Item, * sources may not be handled. * @since 1.473 */ + @SuppressWarnings("unchecked") public void updateByXml(Source source) throws IOException { checkPermission(CONFIGURE); XmlFile configXmlFile = getConfigFile(); @@ -934,12 +934,7 @@ public abstract class AbstractItem extends Actionable implements Loadable, Item, throw new IOException("Expecting " + this.getClass() + " but got " + o.getClass() + " instead"); } - Items.whileUpdatingByXml(new NotReallyRoleSensitiveCallable() { - @Override public Void call() throws IOException { - onLoad(getParent(), getRootDir().getName()); - return null; - } - }); + Items.runWhileUpdatingByXml(() -> onLoad(getParent(), getRootDir().getName())); Jenkins.get().rebuildDependencyGraphAsync(); // if everything went well, commit this new version @@ -967,18 +962,13 @@ public abstract class AbstractItem extends Actionable implements Loadable, Item, } @Override + @SuppressWarnings("unchecked") public void load() throws IOException { checkPermission(CONFIGURE); // try to reflect the changes by reloading getConfigFile().unmarshal(this); - Items.whileUpdatingByXml(new NotReallyRoleSensitiveCallable() { - @Override - public Void call() throws IOException { - onLoad(getParent(), getParent().getItemName(getRootDir(), AbstractItem.this)); - return null; - } - }); + Items.runWhileUpdatingByXml(() -> onLoad(getParent(), getParent().getItemName(getRootDir(), this))); Jenkins.get().rebuildDependencyGraphAsync(); } diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java index b5a0677ec1..3131e67a83 100644 --- a/core/src/main/java/hudson/model/Executor.java +++ b/core/src/main/java/hudson/model/Executor.java @@ -54,7 +54,6 @@ import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; import java.util.Vector; -import java.util.concurrent.Callable; import java.util.concurrent.TimeUnit; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; @@ -365,40 +364,37 @@ public class Executor extends Thread implements ModelObject, IExecutor { SubTask task; // transition from idle to building. // perform this state change as an atomic operation wrt other queue operations - task = Queue.withLock(new Callable<>() { - @Override - public SubTask call() throws Exception { - if (!(owner instanceof Jenkins.MasterComputer)) { - if (!owner.isOnline()) { - resetWorkUnit("went off-line before the task's worker thread was ready to execute"); - return null; - } - if (owner.getNode() == null) { - resetWorkUnit("was removed before the task's worker thread was ready to execute"); - return null; - } + task = Queue.callWithLock(() -> { + if (!(owner instanceof Jenkins.MasterComputer)) { + if (!owner.isOnline()) { + resetWorkUnit("went off-line before the task's worker thread was ready to execute"); + return null; } - // after this point we cannot unwind the assignment of the work unit, if the owner - // is removed or goes off-line then the build will just have to fail. - workUnit.setExecutor(Executor.this); - queue.onStartExecuting(Executor.this); - if (LOGGER.isLoggable(FINE)) - LOGGER.log(FINE, getName() + " grabbed " + workUnit + " from queue"); - SubTask task = workUnit.work; - Executable executable = task.createExecutable(); - if (executable == null) { - String displayName = task instanceof Queue.Task ? ((Queue.Task) task).getFullDisplayName() : task.getDisplayName(); - LOGGER.log(WARNING, "{0} cannot be run (for example because it is disabled)", displayName); + if (owner.getNode() == null) { + resetWorkUnit("was removed before the task's worker thread was ready to execute"); + return null; } - lock.writeLock().lock(); - try { - Executor.this.executable = executable; - } finally { - lock.writeLock().unlock(); - } - workUnit.setExecutable(executable); - return task; } + // after this point we cannot unwind the assignment of the work unit, if the owner + // is removed or goes off-line then the build will just have to fail. + workUnit.setExecutor(Executor.this); + queue.onStartExecuting(Executor.this); + if (LOGGER.isLoggable(FINE)) + LOGGER.log(FINE, getName() + " grabbed " + workUnit + " from queue"); + SubTask _task = workUnit.work; + Executable _executable = _task.createExecutable(); + if (_executable == null) { + String displayName = _task instanceof Queue.Task qt ? qt.getFullDisplayName() : _task.getDisplayName(); + LOGGER.log(WARNING, "{0} cannot be run (for example because it is disabled)", displayName); + } + lock.writeLock().lock(); + try { + executable = _executable; + } finally { + lock.writeLock().unlock(); + } + workUnit.setExecutable(_executable); + return _task; }); Executable executable; lock.readLock().lock(); diff --git a/core/src/main/java/hudson/model/ItemGroupMixIn.java b/core/src/main/java/hudson/model/ItemGroupMixIn.java index 89945267ed..2f1272ea2b 100644 --- a/core/src/main/java/hudson/model/ItemGroupMixIn.java +++ b/core/src/main/java/hudson/model/ItemGroupMixIn.java @@ -48,7 +48,6 @@ import javax.xml.transform.stream.StreamResult; import javax.xml.transform.stream.StreamSource; import jenkins.model.Jenkins; import jenkins.security.ExtendedReadRedaction; -import jenkins.security.NotReallyRoleSensitiveCallable; import jenkins.util.xml.XMLUtils; import org.kohsuke.stapler.StaplerRequest; import org.kohsuke.stapler.StaplerRequest2; @@ -264,11 +263,7 @@ public abstract class ItemGroupMixIn { // reload from the new config final File rootDir = result.getRootDir(); - result = Items.whileUpdatingByXml(new NotReallyRoleSensitiveCallable() { - @Override public T call() throws IOException { - return (T) Items.load(parent, rootDir); - } - }); + result = Items.callWhileUpdatingByXml(() -> (T) Items.load(parent, rootDir)); result.onCopiedFrom(src); add(result); @@ -294,11 +289,7 @@ public abstract class ItemGroupMixIn { XMLUtils.safeTransform(new StreamSource(xml), new StreamResult(configXml)); // load it - TopLevelItem result = Items.whileUpdatingByXml(new NotReallyRoleSensitiveCallable() { - @Override public TopLevelItem call() throws IOException { - return (TopLevelItem) Items.load(parent, dir); - } - }); + TopLevelItem result = Items.callWhileUpdatingByXml(() -> (TopLevelItem) Items.load(parent, dir)); boolean hasCreatePermission = acl.getACL().hasCreatePermission2(Jenkins.getAuthentication2(), parent, result.getDescriptor()); boolean applicableIn = result.getDescriptor().isApplicableIn(parent); diff --git a/core/src/main/java/hudson/model/Items.java b/core/src/main/java/hudson/model/Items.java index fac4ae8a4a..37b7d959c8 100644 --- a/core/src/main/java/hudson/model/Items.java +++ b/core/src/main/java/hudson/model/Items.java @@ -53,6 +53,8 @@ import java.util.function.Predicate; import jenkins.model.DirectlyModifiableTopLevelItemGroup; import jenkins.model.Jenkins; import jenkins.util.MemoryReductionUtil; +import jenkins.util.ThrowingCallable; +import jenkins.util.ThrowingRunnable; import org.apache.commons.io.FileUtils; import org.springframework.security.core.Authentication; @@ -116,6 +118,23 @@ public class Items { } }; + /** + * Runs a block while making {@link #currentlyUpdatingByXml} be temporarily true. + * Use this when you are creating or changing an item. + * @param an error type (may be {@link Error}) + * @param runnable a block, typically running {@link #load} or {@link Item#onLoad} + * @throws T anything {@code runnable} throws + * @since TODO + */ + public static void runWhileUpdatingByXml(ThrowingRunnable runnable) throws T { + updatingByXml.set(true); + try { + runnable.run(); + } finally { + updatingByXml.set(false); + } + } + /** * Runs a block while making {@link #currentlyUpdatingByXml} be temporarily true. * Use this when you are creating or changing an item. @@ -124,6 +143,19 @@ public class Items { * @param callable a block, typically running {@link #load} or {@link Item#onLoad} * @return whatever {@code callable} returned * @throws T anything {@code callable} throws + * @since TODO + */ + public static V callWhileUpdatingByXml(ThrowingCallable callable) throws T { + updatingByXml.set(true); + try { + return callable.call(); + } finally { + updatingByXml.set(false); + } + } + + /** + * Prefer {@link #runWhileUpdatingByXml} or {@link #callWhileUpdatingByXml}. * @since 1.546 */ public static V whileUpdatingByXml(Callable callable) throws T { @@ -138,7 +170,8 @@ public class Items { /** * Checks whether we are in the middle of creating or configuring an item via XML. * Used to determine the {@code newInstance} parameter for {@link Trigger#start}. - * @return true if {@link #whileUpdatingByXml} is currently being called, false for example when merely starting Jenkins or reloading from disk + * @return true if {@link #runWhileUpdatingByXml} or {@link #callWhileUpdatingByXml} is currently being called, + * false for example when merely starting Jenkins or reloading from disk * @since 1.546 */ public static boolean currentlyUpdatingByXml() { diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 8c4a273349..e7fbe83503 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -123,6 +123,8 @@ import jenkins.security.stapler.StaplerAccessibleType; import jenkins.util.AtmostOneTaskExecutor; import jenkins.util.Listeners; import jenkins.util.SystemProperties; +import jenkins.util.ThrowingCallable; +import jenkins.util.ThrowingRunnable; import jenkins.util.Timer; import net.jcip.annotations.GuardedBy; import org.jenkinsci.remoting.RoleChecker; @@ -1275,6 +1277,22 @@ public class Queue extends ResourceController implements Saveable { * Some operations require to be performed with the {@link Queue} lock held. Use one of these methods rather * than locking directly on Queue in order to allow for future refactoring. * @param runnable the operation to perform. + * @throws T if the runnable throws an exception. + * @since TODO + */ + public static void runWithLock(ThrowingRunnable runnable) throws T { + final Jenkins jenkins = Jenkins.getInstanceOrNull(); + // TODO confirm safe to assume non-null and use getInstance() + final Queue queue = jenkins == null ? null : jenkins.getQueue(); + if (queue == null) { + runnable.run(); + } else { + queue._runWithLock(runnable); + } + } + + /** + * Prefer {@link #runWithLock}. * @since 1.592 */ public static void withLock(Runnable runnable) { @@ -1297,6 +1315,21 @@ public class Queue extends ResourceController implements Saveable { * @param the type of exception. * @return the result of the callable. * @throws T the exception of the callable + * @since TODO + */ + public static V callWithLock(ThrowingCallable callable) throws T { + final Jenkins jenkins = Jenkins.getInstanceOrNull(); + // TODO confirm safe to assume non-null and use getInstance() + final Queue queue = jenkins == null ? null : jenkins.getQueue(); + if (queue == null) { + return callable.call(); + } else { + return queue._callWithLock(callable); + } + } + + /** + * Prefer {@link #callWithLock}. * @since 1.592 */ public static V withLock(hudson.remoting.Callable callable) throws T { @@ -1311,13 +1344,7 @@ public class Queue extends ResourceController implements Saveable { } /** - * Some operations require to be performed with the {@link Queue} lock held. Use one of these methods rather - * than locking directly on Queue in order to allow for future refactoring. - * - * @param callable the operation to perform. - * @param the type of return value - * @return the result of the callable. - * @throws Exception if the callable throws an exception. + * Prefer {@link #callWithLock}. * @since 1.592 */ public static V withLock(java.util.concurrent.Callable callable) throws Exception { @@ -1420,6 +1447,25 @@ public class Queue extends ResourceController implements Saveable { condition.signalAll(); } + /** + * Some operations require to be performed with the {@link Queue} lock held. Use one of these methods rather + * than locking directly on Queue in order to allow for future refactoring. + * + * @param runnable the operation to perform. + * @param the type of exception. + * @throws T the exception of the runnable + * @since TODO + */ + @Override + protected void _runWithLock(ThrowingRunnable runnable) throws T { + lock.lock(); + try { + runnable.run(); + } finally { + lock.unlock(); + } + } + /** * Some operations require to be performed with the {@link Queue} lock held. Use one of these methods rather * than locking directly on Queue in order to allow for future refactoring. @@ -1476,6 +1522,27 @@ public class Queue extends ResourceController implements Saveable { } } + /** + * Some operations require to be performed with the {@link Queue} lock held. Use one of these methods rather + * than locking directly on Queue in order to allow for future refactoring. + * + * @param callable the operation to perform. + * @param the type of return value + * @param the type of exception. + * @return the result of the callable. + * @throws T the exception of the callable + * @since TODO + */ + @Override + protected V _callWithLock(ThrowingCallable callable) throws T { + lock.lock(); + try { + return callable.call(); + } finally { + lock.unlock(); + } + } + /** * Some operations require to be performed with the {@link Queue} lock held. Use one of these methods rather * than locking directly on Queue in order to allow for future refactoring. diff --git a/core/src/main/java/hudson/model/ResourceController.java b/core/src/main/java/hudson/model/ResourceController.java index c466fc001a..9e3dcfb209 100644 --- a/core/src/main/java/hudson/model/ResourceController.java +++ b/core/src/main/java/hudson/model/ResourceController.java @@ -31,9 +31,9 @@ import java.util.AbstractCollection; import java.util.Collection; import java.util.Iterator; import java.util.Set; -import java.util.concurrent.Callable; import java.util.concurrent.CopyOnWriteArraySet; -import jenkins.security.NotReallyRoleSensitiveCallable; +import jenkins.util.ThrowingCallable; +import jenkins.util.ThrowingRunnable; /** * Controls mutual exclusion of {@link ResourceList}. @@ -82,32 +82,25 @@ public class ResourceController { */ public void execute(@NonNull Runnable task, final ResourceActivity activity) throws InterruptedException { final ResourceList resources = activity.getResourceList(); - _withLock(new NotReallyRoleSensitiveCallable() { - @Override - public Void call() throws InterruptedException { - while (inUse.isCollidingWith(resources)) { - // TODO revalidate the resource list after re-acquiring lock, for now we just let the build fail - _await(); - } - - // we have a go - inProgress.add(activity); - inUse = ResourceList.union(inUse, resources); - return null; + _runWithLock(() -> { + while (inUse.isCollidingWith(resources)) { + // TODO revalidate the resource list after re-acquiring lock, for now we just let the build fail + _await(); } + + // we have a go + inProgress.add(activity); + inUse = ResourceList.union(inUse, resources); }); try { task.run(); } finally { // TODO if AsynchronousExecution, do that later - _withLock(new Runnable() { - @Override - public void run() { - inProgress.remove(activity); - inUse = ResourceList.union(resourceView); - _signalAll(); - } + _runWithLock(() -> { + inProgress.remove(activity); + inUse = ResourceList.union(resourceView); + _signalAll(); }); } } @@ -122,16 +115,7 @@ public class ResourceController { * gets to call {@link #execute(Runnable, ResourceActivity)}. */ public boolean canRun(final ResourceList resources) { - try { - return _withLock(new Callable<>() { - @Override - public Boolean call() { - return !inUse.isCollidingWith(resources); - } - }); - } catch (Exception e) { - throw new IllegalStateException("Inner callable does not throw exception", e); - } + return _callWithLock(() -> !inUse.isCollidingWith(resources)); } /** @@ -143,16 +127,7 @@ public class ResourceController { * This method is used for reporting what's causing the blockage. */ public Resource getMissingResource(final ResourceList resources) { - try { - return _withLock(new Callable<>() { - @Override - public Resource call() { - return resources.getConflict(inUse); - } - }); - } catch (Exception e) { - throw new IllegalStateException("Inner callable does not throw exception", e); - } + return _callWithLock(() -> resources.getConflict(inUse)); } /** @@ -177,12 +152,24 @@ public class ResourceController { notifyAll(); } + protected void _runWithLock(ThrowingRunnable runnable) throws T { + synchronized (this) { + runnable.run(); + } + } + protected void _withLock(Runnable runnable) { synchronized (this) { runnable.run(); } } + protected V _callWithLock(ThrowingCallable callable) throws T { + synchronized (this) { + return callable.call(); + } + } + protected V _withLock(java.util.concurrent.Callable callable) throws Exception { synchronized (this) { return callable.call(); diff --git a/core/src/main/java/hudson/security/ACL.java b/core/src/main/java/hudson/security/ACL.java index 03d2a21beb..1c4dca9756 100644 --- a/core/src/main/java/hudson/security/ACL.java +++ b/core/src/main/java/hudson/security/ACL.java @@ -41,7 +41,6 @@ import java.util.function.BiFunction; import java.util.stream.Collectors; import jenkins.model.Jenkins; import jenkins.security.NonSerializableSecurityContext; -import jenkins.security.NotReallyRoleSensitiveCallable; import org.acegisecurity.acls.sid.PrincipalSid; import org.acegisecurity.acls.sid.Sid; import org.kohsuke.accmod.Restricted; @@ -438,10 +437,6 @@ public abstract class ACL { } /** - * Safer variant of {@link #impersonate2(Authentication)} that does not require a finally-block. - * @param auth authentication, such as {@link #SYSTEM2} - * @param body an action to run with this alternate authentication in effect (try {@link NotReallyRoleSensitiveCallable}) - * @since 2.266 * @deprecated use try with resources and {@link #as2(Authentication)} */ @Deprecated @@ -455,7 +450,7 @@ public abstract class ACL { } /** - * @deprecated use {@link #impersonate2(Authentication, Callable)} + * @deprecated use try with resources and {@link #as2(Authentication)} * @since 1.587 */ @Deprecated diff --git a/core/src/main/java/hudson/slaves/ComputerRetentionWork.java b/core/src/main/java/hudson/slaves/ComputerRetentionWork.java index 4225685511..b7380eda05 100644 --- a/core/src/main/java/hudson/slaves/ComputerRetentionWork.java +++ b/core/src/main/java/hudson/slaves/ComputerRetentionWork.java @@ -67,17 +67,14 @@ public class ComputerRetentionWork extends AperiodicWork { protected void doAperiodicRun() { final long startRun = System.currentTimeMillis(); for (final Computer c : Jenkins.get().getComputers()) { - Queue.withLock(new Runnable() { - @Override - public void run() { - Node n = c.getNode(); - if (n != null && n.isHoldOffLaunchUntilSave()) - return; - if (!nextCheck.containsKey(c) || startRun > nextCheck.get(c)) { - // at the moment I don't trust strategies to wait more than 60 minutes - final long waitInMins = Math.max(0, Math.min(60, c.getRetentionStrategy().check(c))); - nextCheck.put(c, startRun + TimeUnit.MINUTES.toMillis(waitInMins)); - } + Queue.runWithLock(() -> { + Node n = c.getNode(); + if (n != null && n.isHoldOffLaunchUntilSave()) + return; + if (!nextCheck.containsKey(c) || startRun > nextCheck.get(c)) { + // at the moment I don't trust strategies to wait more than 60 minutes + final long waitInMins = Math.max(0, Math.min(60, c.getRetentionStrategy().check(c))); + nextCheck.put(c, startRun + TimeUnit.MINUTES.toMillis(waitInMins)); } }); } diff --git a/core/src/main/java/hudson/slaves/RetentionStrategy.java b/core/src/main/java/hudson/slaves/RetentionStrategy.java index 82d732cb49..57c69a1a52 100644 --- a/core/src/main/java/hudson/slaves/RetentionStrategy.java +++ b/core/src/main/java/hudson/slaves/RetentionStrategy.java @@ -101,7 +101,7 @@ public abstract class RetentionStrategy implements Describab * @since 1.275 */ public void start(final @NonNull T c) { - Queue.withLock((Runnable) () -> check(c)); + Queue.runWithLock(() -> check(c)); } /** diff --git a/core/src/main/java/hudson/slaves/SimpleScheduledRetentionStrategy.java b/core/src/main/java/hudson/slaves/SimpleScheduledRetentionStrategy.java index 13a267ace6..81e5c9d0f2 100644 --- a/core/src/main/java/hudson/slaves/SimpleScheduledRetentionStrategy.java +++ b/core/src/main/java/hudson/slaves/SimpleScheduledRetentionStrategy.java @@ -211,27 +211,21 @@ public class SimpleScheduledRetentionStrategy extends RetentionStrategy { + if (c.isIdle()) { + LOGGER.log(INFO, "Disconnecting computer {0} as it has finished its scheduled uptime", + new Object[]{c.getName()}); + c.disconnect(OfflineCause + .create(Messages._SimpleScheduledRetentionStrategy_FinishedUpTime())); } }); } else if (c.isIdle() && !c.isAcceptingTasks()) { - Queue.withLock(new Runnable() { - @Override - public void run() { - if (c.isIdle()) { - LOGGER.log(INFO, "Disconnecting computer {0} as it has finished all jobs running when " - + "it completed its scheduled uptime", new Object[]{c.getName()}); - c.disconnect(OfflineCause - .create(Messages._SimpleScheduledRetentionStrategy_FinishedUpTime())); - } + Queue.runWithLock(() -> { + if (c.isIdle()) { + LOGGER.log(INFO, "Disconnecting computer {0} as it has finished all jobs running when " + + "it completed its scheduled uptime", new Object[]{c.getName()}); + c.disconnect(OfflineCause + .create(Messages._SimpleScheduledRetentionStrategy_FinishedUpTime())); } }); } diff --git a/core/src/main/java/hudson/slaves/SlaveComputer.java b/core/src/main/java/hudson/slaves/SlaveComputer.java index 3b12b2447c..c8d79ab779 100644 --- a/core/src/main/java/hudson/slaves/SlaveComputer.java +++ b/core/src/main/java/hudson/slaves/SlaveComputer.java @@ -963,6 +963,7 @@ public class SlaveComputer extends Computer { @Override @SuppressFBWarnings(value = "UR_UNINIT_READ_CALLED_FROM_SUPER_CONSTRUCTOR", justification = "TODO needs triage") + @SuppressWarnings("unchecked") protected void setNode(final Node node) { super.setNode(node); launcher = grabLauncher(node); @@ -971,13 +972,8 @@ public class SlaveComputer extends Computer { // "constructed==null" test is an ugly hack to avoid launching before the object is fully // constructed. if (constructed != null) { - if (node instanceof Slave) { - Queue.withLock(new Runnable() { - @Override - public void run() { - ((Slave) node).getRetentionStrategy().check(SlaveComputer.this); - } - }); + if (node instanceof Slave slave) { + Queue.runWithLock(() -> slave.getRetentionStrategy().check(SlaveComputer.this)); } else { connect(false); } diff --git a/core/src/main/java/jenkins/model/Nodes.java b/core/src/main/java/jenkins/model/Nodes.java index d5f8e90acc..150455e3f3 100644 --- a/core/src/main/java/jenkins/model/Nodes.java +++ b/core/src/main/java/jenkins/model/Nodes.java @@ -196,16 +196,13 @@ public class Nodes implements PersistenceRoot { } catch (IOException | RuntimeException e) { // JENKINS-50599: If persisting the node throws an exception, we need to remove the node from // memory before propagating the exception. - Queue.withLock(new Runnable() { - @Override - public void run() { - nodes.compute(node.getNodeName(), (ignoredNodeName, ignoredNode) -> old); - jenkins.updateComputers(node); - if (old != null) { - jenkins.trimLabels(node, old); - } else { - jenkins.trimLabels(node); - } + Queue.runWithLock(() -> { + nodes.compute(node.getNodeName(), (ignoredNodeName, ignoredNode) -> old); + jenkins.updateComputers(node); + if (old != null) { + jenkins.trimLabels(node, old); + } else { + jenkins.trimLabels(node); } }); throw e; @@ -283,13 +280,10 @@ public class Nodes implements PersistenceRoot { if (oldOne == nodes.get(oldOne.getNodeName())) { // use the queue lock until Nodes has a way of directly modifying a single node. - Queue.withLock(new Runnable() { - @Override - public void run() { - Nodes.this.nodes.remove(oldOne.getNodeName()); - Nodes.this.nodes.put(newOne.getNodeName(), newOne); - newOne.onLoad(Nodes.this, newOne.getNodeName()); - } + Queue.runWithLock(() -> { + Nodes.this.nodes.remove(oldOne.getNodeName()); + Nodes.this.nodes.put(newOne.getNodeName(), newOne); + newOne.onLoad(Nodes.this, newOne.getNodeName()); }); updateNode(newOne, false); if (!newOne.getNodeName().equals(oldOne.getNodeName())) { @@ -318,16 +312,13 @@ public class Nodes implements PersistenceRoot { public void removeNode(final @NonNull Node node) throws IOException { if (node == nodes.get(node.getNodeName())) { AtomicBoolean match = new AtomicBoolean(); - Queue.withLock(new Runnable() { - @Override - public void run() { - Computer c = node.toComputer(); - if (c != null) { - c.recordTermination(); - c.disconnect(OfflineCause.create(hudson.model.Messages._Hudson_NodeBeingRemoved())); - } - match.set(node == nodes.remove(node.getNodeName())); + Queue.runWithLock(() -> { + Computer c = node.toComputer(); + if (c != null) { + c.recordTermination(); + c.disconnect(OfflineCause.create(hudson.model.Messages._Hudson_NodeBeingRemoved())); } + match.set(node == nodes.remove(node.getNodeName())); }); // no need for a full save() so we just do the minimum LOGGER.fine(() -> "deleting " + new File(getRootDir(), node.getNodeName())); @@ -387,21 +378,18 @@ public class Nodes implements PersistenceRoot { } } } - Queue.withLock(new Runnable() { - @Override - public void run() { - newNodes.entrySet().removeIf(stringNodeEntry -> ExtensionList.lookup(NodeListener.class).stream().anyMatch(nodeListener -> { - if (!nodeListener.allowLoad(stringNodeEntry.getValue())) { - LOGGER.log(Level.FINE, () -> "Loading of node " + stringNodeEntry.getKey() + " vetoed by " + nodeListener); - return true; - } - return false; - })); - nodes.entrySet().removeIf(stringNodeEntry -> !(stringNodeEntry.getValue() instanceof EphemeralNode)); - nodes.putAll(newNodes); - jenkins.updateComputerList(); - jenkins.trimLabels(); - } + Queue.runWithLock(() -> { + newNodes.entrySet().removeIf(stringNodeEntry -> ExtensionList.lookup(NodeListener.class).stream().anyMatch(nodeListener -> { + if (!nodeListener.allowLoad(stringNodeEntry.getValue())) { + LOGGER.log(Level.FINE, () -> "Loading of node " + stringNodeEntry.getKey() + " vetoed by " + nodeListener); + return true; + } + return false; + })); + nodes.entrySet().removeIf(stringNodeEntry -> !(stringNodeEntry.getValue() instanceof EphemeralNode)); + nodes.putAll(newNodes); + jenkins.updateComputerList(); + jenkins.trimLabels(); }); } diff --git a/core/src/main/java/jenkins/security/NotReallyRoleSensitiveCallable.java b/core/src/main/java/jenkins/security/NotReallyRoleSensitiveCallable.java index 1d5cf2c3f7..d86fd90493 100644 --- a/core/src/main/java/jenkins/security/NotReallyRoleSensitiveCallable.java +++ b/core/src/main/java/jenkins/security/NotReallyRoleSensitiveCallable.java @@ -1,15 +1,13 @@ package jenkins.security; import hudson.remoting.Callable; +import jenkins.util.ThrowingCallable; import org.jenkinsci.remoting.RoleChecker; /** - * {@link Callable} adapter for situations where Callable is not used for remoting but - * just as a convenient function that has parameterized return value and exception type. - * - * @author Kohsuke Kawaguchi - * @since 1.587 / 1.580.1 + * @deprecated use {@link ThrowingCallable} instead */ +@Deprecated public abstract class NotReallyRoleSensitiveCallable implements Callable { @Override public void checkRoles(RoleChecker checker) throws SecurityException { diff --git a/core/src/main/java/jenkins/util/ThrowingCallable.java b/core/src/main/java/jenkins/util/ThrowingCallable.java new file mode 100644 index 0000000000..1e519ed920 --- /dev/null +++ b/core/src/main/java/jenkins/util/ThrowingCallable.java @@ -0,0 +1,54 @@ +/* + * The MIT License + * + * Copyright 2025 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package jenkins.util; + +import java.io.Serializable; +import jenkins.security.NotReallyRoleSensitiveCallable; +import org.jenkinsci.remoting.RoleSensitive; + +/** + * A task that returns a result and may throw an exception. + * Similar to {@link java.util.concurrent.Callable} except that the exception type can be constrained. + * Similar to {@link hudson.remoting.Callable} or {@link NotReallyRoleSensitiveCallable} except + *
    + *
  • It is not {@link Serializable}, which would cause SpotBugs to complain about captured local variables. + *
  • It does not have the {@link RoleSensitive#checkRoles} so it can be a {@link FunctionalInterface}. + *
+ * Similar to {@link ThrowingRunnable} but returns a value. + * @param the return type + * @param the checked exception type, or might be {@link RuntimeException} + * @since TODO + */ +@FunctionalInterface +public interface ThrowingCallable { + + /** + * Computes a result, or throws an exception if unable to do so. + * @return computed result + * @throws T if unable to compute a result + */ + V call() throws T; + +} diff --git a/core/src/main/java/jenkins/util/ThrowingRunnable.java b/core/src/main/java/jenkins/util/ThrowingRunnable.java new file mode 100644 index 0000000000..e1e80cbcae --- /dev/null +++ b/core/src/main/java/jenkins/util/ThrowingRunnable.java @@ -0,0 +1,42 @@ +/* + * The MIT License + * + * Copyright 2025 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package jenkins.util; + +/** + * Represents an operation that does not return a result. + * Similar to {@link Runnable} but can throw a checked exception. + * Similar to {@link ThrowingCallable} but does not return a value. + * @param the checked exception type, or might be {@link RuntimeException} + */ +@FunctionalInterface +public interface ThrowingRunnable { + + /** + * Runs this operation. + * @throws T if unable to run + */ + void run() throws T; + +}