diff --git a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java index 591274d2..f35310de 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java @@ -181,7 +181,7 @@ public String url() { * Flag for whether or not the build has completed somehow. * This was previously a transient field, so we may need to recompute in {@link #onLoad} based on {@link FlowExecution#isComplete}. */ - Boolean completed; // Non-private for testing + volatile Boolean completed; // Non-private for testing /** Protects the access to logsToCopy, completed, and branchNameCache that are used in the logCopy process */ private transient Object logCopyGuard = new Object(); @@ -508,7 +508,7 @@ private String key() { new XmlFile(XSTREAM,new File(getRootDir(),"build.xml")).unmarshal(this); } - @Override protected void onLoad() { + @Override protected void onLoad() { // Here there be DRAGONS: due to lazy-loading and subtle threading risks, be very cautious! try { synchronized (getLogCopyGuard()) { if (executionLoaded) { @@ -572,6 +572,20 @@ private String key() { } } + private void endExecutionTask() { + try { + Executor executor = getExecutor(); + if (executor != null) { + AsynchronousExecution asynchronousExecution = executor.getAsynchronousExecution(); + if (asynchronousExecution != null) { + asynchronousExecution.completed(null); + } + } + } catch (Exception ex) { + LOGGER.log(Level.WARNING, "Error when trying to end the FlyWeightTask for run "+this, ex); + } + } + /** 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) { boolean nullListener = false; @@ -580,6 +594,7 @@ private void finish(@Nonnull Result r, @CheckForNull Throwable t) { setResult(r); completed = Boolean.TRUE; duration = Math.max(0, System.currentTimeMillis() - getStartTimeInMillis()); + } try { LOGGER.log(Level.INFO, "{0} completed: {1}", new Object[]{toString(), getResult()}); @@ -605,7 +620,8 @@ private void finish(@Nonnull Result r, @CheckForNull Throwable t) { } listener = null; } - saveWithoutFailing(); // TODO useless if we are inside a BulkChange + saveWithoutFailing(); + endExecutionTask(); Timer.get().submit(() -> { try { getParent().logRotate(); @@ -614,7 +630,8 @@ private void finish(@Nonnull Result r, @CheckForNull Throwable t) { } }); onEndBuilding(); - } finally { // Ensure this is ALWAYS removed from FlowExecutionList + } finally { // Ensure this is ALWAYS removed from FlowExecutionList and finished + endExecutionTask(); // Just in case an exception was thrown above FlowExecutionList.get().unregister(new Owner(this)); } @@ -1109,8 +1126,9 @@ private void saveWithoutFailing() { @Override public void save() throws IOException { - - if(BulkChange.contains(this)) return; + /* Checking for completion ensures the final save can complete. + */ + if(BulkChange.contains(this) && completed != Boolean.TRUE) return; File loc = new File(getRootDir(),"build.xml"); XmlFile file = new XmlFile(XSTREAM,loc); @@ -1121,23 +1139,9 @@ public void save() throws IOException { isAtomic = hint.isAtomicWrite(); } - boolean completeAsynchronousExecution = false; - try { - synchronized (this) { - completeAsynchronousExecution = Boolean.TRUE.equals(completed); - PipelineIOUtils.writeByXStream(this, loc, XSTREAM2, isAtomic); - SaveableListener.fireOnChange(this, file); - } - } finally { - if (completeAsynchronousExecution) { - Executor executor = getExecutor(); - if (executor != null) { - AsynchronousExecution asynchronousExecution = executor.getAsynchronousExecution(); - if (asynchronousExecution != null) { - asynchronousExecution.completed(null); - } - } - } + synchronized (this) { + PipelineIOUtils.writeByXStream(this, loc, XSTREAM2, isAtomic); + SaveableListener.fireOnChange(this, file); } } } diff --git a/src/test/java/org/jenkinsci/plugins/workflow/job/CpsPersistenceTest.java b/src/test/java/org/jenkinsci/plugins/workflow/job/CpsPersistenceTest.java index e963f7ec..13ce2edf 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/job/CpsPersistenceTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/job/CpsPersistenceTest.java @@ -79,9 +79,8 @@ private static Stack getCpsBlockStartNodes(CpsFlowExecution exec /** Verifies all the assumptions about a cleanly finished build. */ static void assertCompletedCleanly(WorkflowRun run) throws Exception { - if (run.isBuilding()) { - System.out.println("Run initially building, going to wait a second to see if it finishes, run="+run); - Thread.sleep(1000); + while (run.isBuilding()) { + Thread.sleep(100); // Somewhat variable speeds } Assert.assertFalse(run.isBuilding()); Assert.assertNotNull(run.getResult()); @@ -103,7 +102,9 @@ static void assertCompletedCleanly(WorkflowRun run) throws Exception { Stack starts = getCpsBlockStartNodes(cpsExec); Assert.assertTrue(starts == null || starts.isEmpty()); Thread.sleep(1000); // TODO seems to be flaky - Assert.assertFalse(cpsExec.blocksRestart()); + while (cpsExec.blocksRestart()) { + Thread.sleep(100); + } } else { System.out.println("WARNING: no FlowExecutionForBuild"); } diff --git a/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java b/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java index a8a3ea72..0f552fec 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java @@ -129,7 +129,6 @@ public class WorkflowRunRestartTest { WorkflowJob p = r.jenkins.getItemByFullName("p", WorkflowJob.class); WorkflowRun b = p.getBuildByNumber(1); assertNotNull(b.asFlowExecutionOwner()); - assertNull(b.execution.getOwner()); assertFalse(b.executionLoaded); assertTrue(b.completed); assertFalse(b.isBuilding());