Merge pull request #159 from jglick/simplify-WorkflowRun

[JENKINS-27531] Simplified fix
Originally-Committed-As: 307f9e1cf6113b48df9480c78244ec0029c33c03

job/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

@@ -42,7 +42,6 @@
 import hudson.model.Queue;
 import hudson.model.Result;
 import hudson.model.Run;
-import hudson.model.RunMap;
 import hudson.model.StreamBuildListener;
 import hudson.model.TaskListener;
 import hudson.model.listeners.RunListener;
@@ -57,7 +56,6 @@
 import java.io.IOException;
 import java.io.OutputStream;
 import java.io.PrintStream;
-import java.lang.reflect.Field;
 import java.nio.charset.Charset;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -168,7 +166,6 @@ public WorkflowRun(WorkflowJob job, File dir) throws IOException {
         if (!firstTime) {
             throw sleep();
         }
-        // Some code here copied from execute(RunExecution), but subsequently modified quite a bit.
         try {
             onStartBuilding();
             OutputStream logger = new FileOutputStream(getLogFile());
@@ -178,8 +175,7 @@ public WorkflowRun(WorkflowJob job, File dir) throws IOException {
             updateSymlinks(listener);
             FlowDefinition definition = getParent().getDefinition();
             if (definition == null) {
-                listener.error("No flow definition, cannot run");
-                return;
+                throw new AbortException("No flow definition, cannot run");
             }
             Owner owner = new Owner(this);
 
@@ -195,12 +191,7 @@ public WorkflowRun(WorkflowJob job, File dir) throws IOException {
             execution = newExecution;
             executionPromise.set(newExecution);
         } catch (Throwable x) {
-            if (listener == null) {
-                LOGGER.log(Level.WARNING, this + " failed to start", x);
-            } else {
-                x.printStackTrace(listener.error("failed to start build"));
-            }
-            setResult(Result.FAILURE);
+            finish(Result.FAILURE, x);
             executionPromise.setException(x);
             return;
         }
@@ -365,42 +356,8 @@ protected void eol(byte[] b, int len) throws IOException {
 
     private static final Map<String,WorkflowRun> LOADING_RUNS = new HashMap<String,WorkflowRun>();
 
-    /**
-     * Same as {@link Run#getId} except it works before the run has been loaded from disk.
-     * TODO JENKINS-27531 this logic should be handled directly in Run.getId() instead.
-     */
-    private static String getId(WorkflowRun r) {
-        String id = r.getId();
-        Class<?> runIdMigratorC;
-        try {
-            runIdMigratorC = Class.forName("jenkins.model.RunIdMigrator");
-        } catch (ClassNotFoundException x) {
-            // 1.596 or earlier, so the ID is fine.
-            return id;
-        }
-        try {
-            RunMap<WorkflowRun> runMap = r.getParent()._getRuns();
-            Field runIdMigratorF = RunMap.class.getField("runIdMigrator");
-            Object runIdMigratorO = runIdMigratorF.get(runMap);
-            Field idToNumberF = runIdMigratorC.getDeclaredField("idToNumber");
-            idToNumberF.setAccessible(true);
-            Map<String,Integer> idToNumberO = (Map<String,Integer>) idToNumberF.get(runIdMigratorO);
-            int n = r.getNumber();
-            for (Map.Entry<String,Integer> entry : idToNumberO.entrySet()) {
-                if (entry.getValue().equals(n)) {
-                    id = entry.getKey();
-                    LOGGER.log(Level.FINE, "recovered legacy ID {0} for {1}", new Object[] {id, r});
-                    return id;
-                }
-            }
-        } catch (Exception x) {
-            LOGGER.log(Level.WARNING, null, x);
-        }
-        return id;
-    }
-
     private String key() {
-        return getParent().getFullName() + '/' + getId(this);
+        return getParent().getFullName() + '/' + getId();
     }
 
     /** Hack to allow {@link #execution} to use an {@link Owner} referring to this run, even when it has not yet been loaded. */
@@ -460,21 +417,25 @@ private String key() {
         }
     }
 
-    private void finish(Result r) {
+    /** Handles normal build completion (including errors) but also handles the case that the flow did not even start correctly, for example due to an error in {@link FlowExecution#start}. */
+    private void finish(@Nonnull Result r, @CheckForNull Throwable t) {
         setResult(r);
         LOGGER.log(Level.INFO, "{0} completed: {1}", new Object[] {this, getResult()});
         // TODO set duration
-        RunListener.fireCompleted(WorkflowRun.this, listener);
-        Throwable t = execution.getCauseOfFailure();
-        if (t instanceof AbortException) {
-            listener.error(t.getMessage());
-        } else if (t instanceof FlowInterruptedException) {
-            ((FlowInterruptedException) t).handle(this, listener);
-        } else if (t != null) {
-            t.printStackTrace(listener.getLogger());
-        }
-        listener.finished(getResult());
-        listener.closeQuietly();
+        if (listener == null) {
+            LOGGER.log(Level.WARNING, this + " failed to start", t);
+        } else {
+            RunListener.fireCompleted(WorkflowRun.this, listener);
+            if (t instanceof AbortException) {
+                listener.error(t.getMessage());
+            } else if (t instanceof FlowInterruptedException) {
+                ((FlowInterruptedException) t).handle(this, listener);
+            } else if (t != null) {
+                t.printStackTrace(listener.getLogger());
+            }
+            listener.finished(getResult());
+            listener.closeQuietly();
+        }
         logsToCopy = null;
         duration = Math.max(0, System.currentTimeMillis() - getStartTimeInMillis());
         try {
@@ -484,11 +445,12 @@ private void finish(Result r) {
             LOGGER.log(Level.WARNING, null, x);
         }
         onEndBuilding();
-        assert completed != null;
-        synchronized (completed) {
-            completed.set(true);
+        if (completed != null) {
+            synchronized (completed) {
+                completed.set(true);
+            }
         }
-        FlowExecutionList.get().unregister(execution.getOwner());
+        FlowExecutionList.get().unregister(new Owner(this));
     }
 
     /**
@@ -614,7 +576,7 @@ private String key() {
                 synchronized (LOADING_RUNS) {
                     candidate = LOADING_RUNS.get(key());
                 }
-                if (candidate != null && candidate.getParent().getFullName().equals(job) && getId(candidate).equals(id)) {
+                if (candidate != null && candidate.getParent().getFullName().equals(job) && candidate.getId().equals(id)) {
                     run = candidate;
                 } else {
                     Jenkins jenkins = Jenkins.getInstance();
@@ -692,7 +654,7 @@ public int hashCode() {
 
             logNodeMessage(node, "Running: " + node.getDisplayName());
             if (node instanceof FlowEndNode) {
-                finish(((FlowEndNode) node).getResult());
+                finish(((FlowEndNode) node).getResult(), execution.getCauseOfFailure());
             } else {
                 try {
                     save();