From 4521fef5822dd23c5713fede0a7faf82be73a4b4 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 25 Aug 2023 13:21:56 +0200 Subject: [PATCH 1/5] computeNext() should not cause a side-effect in the normal case --- .../plugins/workflow/flow/FlowExecutionList.java | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java b/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java index 17f25f4a..f145732b 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java @@ -80,7 +80,7 @@ public FlowExecutionList() { */ @Override public Iterator iterator() { - return new AbstractIterator() { + return new AbstractIterator<>() { final Iterator base = runningTasks.iterator(); @Override @@ -88,12 +88,7 @@ protected FlowExecution computeNext() { while (base.hasNext()) { FlowExecutionOwner o = base.next(); try { - FlowExecution e = o.get(); - if (e.isComplete()) { - unregister(o); - } else { - return e; - } + return o.get(); } catch (Throwable e) { LOGGER.log(Level.FINE, "Failed to load " + o + ". Unregistering", e); unregister(o); From 1589b4a9a651994650d37c0797dc7b78dd6ec8fd Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 25 Aug 2023 15:24:23 +0200 Subject: [PATCH 2/5] Still need to return a non-completed FlowExecution --- .../jenkinsci/plugins/workflow/flow/FlowExecutionList.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java b/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java index f145732b..138f9f86 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java @@ -88,7 +88,10 @@ protected FlowExecution computeNext() { while (base.hasNext()) { FlowExecutionOwner o = base.next(); try { - return o.get(); + FlowExecution e = o.get(); + if (!e.isComplete()) { + return e; + } } catch (Throwable e) { LOGGER.log(Level.FINE, "Failed to load " + o + ". Unregistering", e); unregister(o); From e75eec2ef0387843d50fd0bae4d968fbdbc938ab Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 25 Aug 2023 15:26:36 +0200 Subject: [PATCH 3/5] On load, unregister any complete FlowExecution to clean up stale entries --- .../jenkinsci/plugins/workflow/flow/FlowExecutionList.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java b/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java index 138f9f86..d6161d87 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java @@ -208,8 +208,11 @@ public static class ItemListenerImpl extends ItemListener { public void onLoaded() { FlowExecutionList list = FlowExecutionList.get(); for (final FlowExecution e : list) { - // The call to FlowExecutionOwner.get in the implementation of iterator() is sufficent to load the Pipeline. + // The call to FlowExecutionOwner.get in the implementation of iterator() is sufficient to load the Pipeline. LOGGER.log(Level.FINE, "Eagerly loaded {0}", e); + if (e.isComplete()) { + list.unregister(e.getOwner()); + } } list.resumptionComplete = true; } From f16659046459dbe9b90088f52080df3d8bd4f5dc Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 25 Aug 2023 16:58:07 +0200 Subject: [PATCH 4/5] Use a different method to iterate while loading so that we can discard complete FlowExecutions. --- .../workflow/flow/FlowExecutionList.java | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java b/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java index d6161d87..36463310 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java @@ -39,7 +39,6 @@ import java.util.logging.Logger; import org.jenkinsci.plugins.workflow.graph.FlowNode; import org.jenkinsci.plugins.workflow.graphanalysis.LinearBlockHoppingScanner; -import org.jvnet.hudson.reactor.Milestone; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.Beta; @@ -206,16 +205,24 @@ public boolean isResumptionComplete() { public static class ItemListenerImpl extends ItemListener { @Override public void onLoaded() { - FlowExecutionList list = FlowExecutionList.get(); - for (final FlowExecution e : list) { - // The call to FlowExecutionOwner.get in the implementation of iterator() is sufficient to load the Pipeline. + FlowExecutionList.get().resume(); + } + } + + private void resume() { + for (final FlowExecutionOwner o : runningTasks) { + try { + FlowExecution e = o.get(); LOGGER.log(Level.FINE, "Eagerly loaded {0}", e); if (e.isComplete()) { - list.unregister(e.getOwner()); + unregister(o); } + } catch (IOException ex) { + LOGGER.log(Level.FINE, "Failed to load " + o + ". Unregistering", ex); + unregister(o); } - list.resumptionComplete = true; } + resumptionComplete = true; } /** From ca179a265250a67d15be16f38a3dd3c7a4c96654 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 25 Aug 2023 17:06:51 +0200 Subject: [PATCH 5/5] Remove from FlowExecutionList using iterator#remove to be safe --- .../plugins/workflow/flow/FlowExecutionList.java | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java b/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java index 36463310..d1c5ae10 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java @@ -210,18 +210,26 @@ public void onLoaded() { } private void resume() { - for (final FlowExecutionOwner o : runningTasks) { + boolean needSave = false; + for (var it = runningTasks.iterator(); it.hasNext(); ) { + var o = it.next(); try { FlowExecution e = o.get(); LOGGER.log(Level.FINE, "Eagerly loaded {0}", e); if (e.isComplete()) { - unregister(o); + LOGGER.log(Level.FINE, "Unregistering completed " + o, e); + it.remove(); + needSave = true; } } catch (IOException ex) { LOGGER.log(Level.FINE, "Failed to load " + o + ". Unregistering", ex); - unregister(o); + it.remove(); + needSave = true; } } + if (needSave) { + saveLater(); + } resumptionComplete = true; }