Yet more fixes #96

svanoort · 2018-04-28T00:08:23Z

MMMMM-multifix for (among others)

JENKINS-50888 - solved by fixing lazy-load issues where the execution's onLoad method isn't called.
JENKINS-50199 - builds not showing as done and resuming
Runs not showing as completed and hanging around as zombies if the FlowNodes can't load and they fail (because the GraphListener wasn't attached to call finish, the build was never marked completed). Also accounts for some with the 'blocksRestart' and similar.
Lazy load changes removed protection against double onLoad calls, restoring that
Fix various issues around lazy load and ensure build's result is set so it gets marked complete.

…LazyLoad

…getter

…missing nodes

…hout preventing finish of onLoad method

…nd need to use getter for initialization

…witch to a save utility

jglick

Some minor comments, but from my limited understanding I do not see anything broken here.

jglick · 2018-05-01T14:48:55Z

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

@@ -439,7 +442,25 @@ public void doTerm() {
            return;
        }
        final Throwable x = new FlowInterruptedException(Result.ABORTED);
-        Futures.addCallback(execution.getCurrentExecutions(/* cf. JENKINS-26148 */true), new FutureCallback<List<StepExecution>>() {
+        FlowExecution exec = getExecution();
+        if (exec == null) { // Already dead, just make sure statuses reflect that.


Seems like this check belongs more in doKill, but OK.

nah, doKill need not check - it's GOING TO null it, guaranteed. 😄

jglick · 2018-05-01T14:51:18Z

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

+                } else { // Remove the logs to copy - execution too broken to fetch nodes
+                    logsToCopy.remove(id);
+                    modified = true;
+                    continue;


I think you mean break here.

Actually I think you meant to pull the getExecution() call outside the logsToCopy loop, just like the original “broken somehow” code just skipped the rest of this method.

Good point, doing an even better version of that.

jglick · 2018-05-01T14:53:21Z

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

                super.onLoad();

+                if (completed == Boolean.TRUE && result == null) {
+                    LOGGER.log(Level.FINE, "Completed build with no result set, defaulting to failure for"+this);


BTW

LOGGER.log(Level.FINE, "Completed build with no result set, defaulting to failure for {0}", this);

(NetBeans at least will offer a code hint for this.)

jglick · 2018-05-01T14:56:32Z

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

+                if (result == null) {
+                    setResult(Result.FAILURE);
+                }
+                LOGGER.log(Level.WARNING, "Nulling out FlowExecution due to error in build "+this.getFullDisplayName(), x);


For log messages, prefer Run.fullName (or simply Run.toString ~ this) over Run.fullDisplayName.

Good catch.

jglick · 2018-05-01T14:57:51Z

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

+    private final class FailOnLoadListener implements GraphListener {
+        @Override public void onNewHead(FlowNode node) {
+            if (node instanceof FlowEndNode) {
+                Thread finishThread = new Thread(new Runnable() {


Timer.get().schedule(() -> { synchronized (getLogCopyGuard()) { finish(((FlowEndNode) node).getResult(), execution != null ? execution.getCauseOfFailure() : null); } }, 1, TimeUnit.SECONDS);

Good point -- should have done it that way initially, brain fart there.

abayer · 2018-05-01T15:11:49Z

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

-        Futures.addCallback(execution.getCurrentExecutions(/* cf. JENKINS-26148 */true), new FutureCallback<List<StepExecution>>() {
+        FlowExecution exec = getExecution();
+        if (exec == null) { // Already dead, just make sure statuses reflect that.
+            synchronized (getLogCopyGuard()) {


Any possibility of some comments explaining the logic here?

@abayer done!

needs re-review for changes

rsandell

🐝

jglick

With the same caveats.

jglick · 2018-05-02T17:05:59Z

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

@@ -721,7 +724,7 @@ private String key() {
                super.onLoad();

                if (completed == Boolean.TRUE && result == null) {
-                    LOGGER.log(Level.FINE, "Completed build with no result set, defaulting to failure for"+this);
+                    LOGGER.log(Level.FINE, "Completed build with no result set, defaulting to failure for "+this);


Better style FWIW:

LOGGER.log(Level.FINE, "Completed build with no result set, defaulting to failure for {0}", this);

itdependsnetworks · 2018-05-02T23:04:13Z

@svanoort seem to be getting issues now:

	at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1890)
	at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1492)
	at sun.net.www.protocol.http.HttpURLConnection.getHeaderField(HttpURLConnection.java:3000)
	at java.net.URLConnection.getHeaderFieldLong(URLConnection.java:629)
	at java.net.URLConnection.getContentLengthLong(URLConnection.java:501)
	at java.net.URLConnection.getContentLength(URLConnection.java:485)
	at hudson.model.UpdateCenter$UpdateCenterConfiguration.download(UpdateCenter.java:1126)
Caused: java.io.FileNotFoundException: http://archives.jenkins-ci.org/plugins/workflow-job/2.21/workflow-job.hpi
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at sun.net.www.protocol.http.HttpURLConnection$10.run(HttpURLConnection.java:1944)
	at sun.net.www.protocol.http.HttpURLConnection$10.run(HttpURLConnection.java:1939)
	at java.security.AccessController.doPrivileged(Native Method)
	at sun.net.www.protocol.http.HttpURLConnection.getChainedException(HttpURLConnection.java:1938)
	at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1508)
	at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1492)
	at hudson.model.UpdateCenter$UpdateCenterConfiguration.download(UpdateCenter.java:1139)
Caused: java.io.IOException: Failed to load http://updates.jenkins-ci.org/download/plugins/workflow-job/2.21/workflow-job.hpi to /var/lib/jenkins/plugins/workflow-job.jpi.tmp
	at hudson.model.UpdateCenter$UpdateCenterConfiguration.download(UpdateCenter.java:1146)
Caused: hudson.util.IOException2: Failed to download from http://updates.jenkins-ci.org/download/plugins/workflow-job/2.21/workflow-job.hpi (redirected to: http://archives.jenkins-ci.org/plugins/workflow-job/2.21/workflow-job.hpi)
	at hudson.model.UpdateCenter$UpdateCenterConfiguration.download(UpdateCenter.java:1172)
	at hudson.model.UpdateCenter$DownloadJob._run(UpdateCenter.java:1677)
	at hudson.model.UpdateCenter$InstallationJob._run(UpdateCenter.java:1874)
	at hudson.model.UpdateCenter$DownloadJob.run(UpdateCenter.java:1651)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at hudson.remoting.AtmostOneThreadExecutor$Worker.run(AtmostOneThreadExecutor.java:112)
	at java.lang.Thread.run(Thread.java:748)

svanoort added 9 commits April 24, 2018 21:59

Set build result if we cannot load FlowExecution

e303ff9

Handle completed with no result set and note double onLoad calls

9964f77

Ensure the GraphListener is attached to the FlowExecution as part of …

1810a11

…LazyLoad

Avoid isInProgress touching an un-initialized FlowExecution by using …

430daf3

…getter

Handle an edge case with completed but corrupted builds

436a470

Do not attach listeners to a completed build just in case we rebuild …

8d82b4a

…missing nodes

Hacky but functional way to ensure build gets marked as completed wit…

ee625dd

…hout preventing finish of onLoad method

Cover a few more places where we're fetching the execution directly a…

e198823

…nd need to use getter for initialization

Fix some subtle NPEs and synchronization issues found by FindBugs + s…

b48c1b5

…witch to a save utility

svanoort changed the title ~~[WIP] More fixes~~ Yet more fixes May 1, 2018

svanoort requested review from jglick, abayer, kshultzCB and rsandell and removed request for jglick May 1, 2018 14:43

jglick previously approved these changes May 1, 2018

View reviewed changes

abayer previously approved these changes May 1, 2018

View reviewed changes

svanoort added 3 commits May 1, 2018 17:51

Synchronize mutation of execution and also add explanatory comment

60c35f4

Clean up copyLogs handling of null FlowExecution per review comments

ed5a4d5

Cleanup the FailOnLoadListener logic

2d7235b

svanoort requested review from abayer and jglick May 1, 2018 22:05

rsandell approved these changes May 2, 2018

View reviewed changes

abayer approved these changes May 2, 2018

View reviewed changes

jglick approved these changes May 2, 2018

View reviewed changes

svanoort merged commit aad1756 into jenkinsci:master May 2, 2018

svanoort deleted the more-fixes branch May 2, 2018 21:01

jglick mentioned this pull request Nov 7, 2024

Always call super from WorkflowRun.onLoad #485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yet more fixes #96

Yet more fixes #96

svanoort commented Apr 28, 2018 •

edited

Loading

jglick left a comment

jglick May 1, 2018

svanoort May 1, 2018

jglick May 1, 2018

svanoort May 1, 2018

jglick May 1, 2018

jglick May 1, 2018

svanoort May 1, 2018

jglick May 1, 2018

svanoort May 1, 2018

abayer May 1, 2018

svanoort May 1, 2018

rsandell left a comment

jglick left a comment

jglick May 2, 2018

itdependsnetworks commented May 2, 2018 •

edited

Loading

Yet more fixes #96

Yet more fixes #96

Conversation

svanoort commented Apr 28, 2018 • edited Loading

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsandell left a comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itdependsnetworks commented May 2, 2018 • edited Loading

svanoort commented Apr 28, 2018 •

edited

Loading

itdependsnetworks commented May 2, 2018 •

edited

Loading