Skip to content
Permalink
Browse files

[JENKINS-43653] - Ensure AbstractItem#delete() NPE safety when checki…

…ng executors (#2854)

* [JENKINS-43653] - Ensure AbstractItem#delete() NPE and RTE safety when checking executors.

This change adds missing NPE checks and also improves handling of Executables#getParentOf(), which may throw undocumented Runtime exceptions if Executable uses core API below 1.377 and does not implement getParent(). Executor#stop() has been also modified to explicitly handle the issue though there is still the same issue with estimated execution times.

* [JENKINS-43653] - Address comments from @jglick, restrict newly introduced API

* [JENKINS-43653] - Address comments from @stephenc

* [JENKINS-43653] - Cleanup "unrelevant" changes to make @stephenc and @jglick happy

* [JENKINS-43653] - Cleanup the leftover

* [JENKINS-43653] - Drop the changes in Executables
  • Loading branch information
oleg-nenashev committed Apr 23, 2017
1 parent cdb45dd commit 132ca7cffe9162b4b04bf52d2cf13a44c0f902e0
@@ -31,6 +31,7 @@
import hudson.Functions;
import hudson.BulkChange;
import hudson.cli.declarative.CLIResolver;
import hudson.model.Queue.Executable;
import hudson.model.listeners.ItemListener;
import hudson.model.listeners.SaveableListener;
import hudson.model.queue.Tasks;
@@ -87,6 +88,8 @@
import javax.xml.transform.stream.StreamSource;

import static hudson.model.queue.Executables.getParentOf;
import hudson.model.queue.SubTask;
import java.lang.reflect.InvocationTargetException;
import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
import org.apache.commons.io.FileUtils;
import org.kohsuke.accmod.Restricted;
@@ -620,9 +623,12 @@ public void delete() throws IOException, InterruptedException {
Map<Executor, Queue.Executable> buildsInProgress = new LinkedHashMap<>();
for (Computer c : Jenkins.getInstance().getComputers()) {
for (Executor e : c.getAllExecutors()) {
WorkUnit workUnit = e.getCurrentWorkUnit();
if (workUnit != null) {
Item item = Tasks.getItemOf(getParentOf(workUnit.getExecutable()));
final WorkUnit workUnit = e.getCurrentWorkUnit();
final Executable executable = workUnit != null ? workUnit.getExecutable() : null;
final SubTask subtask = executable != null ? getParentOf(executable) : null;

if (subtask != null) {
Item item = Tasks.getItemOf(subtask);
if (item != null) {
while (item != null) {
if (item == this) {
@@ -511,6 +511,7 @@ public void completedAsynchronous(@CheckForNull Throwable error) {
* null if the executor is idle.
*/
@Exported
@CheckForNull
public WorkUnit getCurrentWorkUnit() {
lock.readLock().lock();
try {
@@ -36,10 +36,14 @@
* @author Kohsuke Kawaguchi
*/
public class Executables {

/**
* Due to the return type change in {@link Executable}, the caller needs a special precaution now.
* Due to the return type change in {@link Executable} in 1.377, the caller needs a special precaution now.
* @param e Executable
* @return Discovered subtask
*/
public static @Nonnull SubTask getParentOf(Executable e) {
public static @Nonnull SubTask getParentOf(@Nonnull Executable e)
throws Error, RuntimeException {
try {
return e.getParent();
} catch (AbstractMethodError _) {
@@ -77,6 +81,7 @@ public static long getEstimatedDurationFor(@CheckForNull Executable e) {
try {
return e.getEstimatedDuration();
} catch (AbstractMethodError error) {
// TODO: according to the code above, e.getparent() may fail. The method also needs to be reworked
return e.getParent().getEstimatedDuration();
}
}
@@ -79,6 +79,7 @@ public void setExecutor(@CheckForNull Executor e) {
/**
* If the execution has already started, return the executable that was created.
*/
@CheckForNull
public Executable getExecutable() {
return executable;
}

0 comments on commit 132ca7c

Please sign in to comment.
You can’t perform that action at this time.