[FIXED JENKINS-26964] Handle AbortException right #1577

KostyaSha · 2015-02-20T16:01:47Z

jtnord · 2015-02-23T10:29:01Z

core/src/main/java/hudson/model/AbstractBuild.java

-            e.printStackTrace(listener.error(msg));
+            if (!(e instanceof AbortException)){
+                e.printStackTrace(listener.error(msg));
+            }
            LOGGER.log(WARNING, msg, e);


why print the stack trace in the log for Aborts?

Read code again.

I'm not talking about the build log (listener) but the Jenkins log Logger.log(..) which is called outside the if() statement

Hm, i can move it to if.

KostyaSha · 2015-02-23T20:04:54Z

Fixed comment, fixup'ed into one commit.

ikedam · 2015-02-28T05:22:10Z

How about overriding AbortException#printStacktrace()?

It can print the message without the stacktrace.
No need to handle AbortException everywhere in Jenkins.

KostyaSha · 2015-02-28T12:38:35Z

@ikedam 👎

jtnord · 2015-03-01T09:24:12Z

👍

oleg-nenashev · 2015-03-02T16:31:09Z

@ikedam
I would print AbortException contents to the log in any case. Probably, for code>AbortException stack-traces it makes sense to change the logging level to INFO or FINE in such case

KostyaSha · 2015-03-02T18:10:03Z

@oleg-nenashev to which log? No need to print stacktrace into listener, this is known and handled exception.

orrc · 2015-03-28T02:29:21Z

test/src/test/java/hudson/model/FreestyleJobPublisherTest.java

+        p.getPublishersList().add(new ArtifactArchiver("result.txt", "", false));
+
+        FreeStyleBuild b = p.scheduleBuild2(0).get();
+        assertEquals("Build must fail, because we used FalsePublisher", b.getResult(), Result.FAILURE);


FalsePublisher -> AbortExceptionPublisher

orrc · 2015-03-28T02:30:00Z

FWIW, the testFreestyleWithFalsePublisher test case fails on my machine.

KostyaSha · 2015-03-28T02:32:59Z

@orrc interesting, maybe something changed in core. I will try re-trigger PR-build

kohsuke · 2015-04-29T19:49:59Z

👍

jglick · 2015-04-29T19:51:39Z

core/src/main/java/hudson/model/AbstractBuild.java

@@ -733,8 +734,10 @@ protected final boolean performAllBuildSteps(BuildListener listener, Iterable<?

        private void reportError(BuildStep bs, Throwable e, BuildListener listener, boolean phase) {
            String msg = "Publisher " + bs.getClass().getName() + " aborted due to exception";
-            e.printStackTrace(listener.error(msg));
-            LOGGER.log(WARNING, msg, e);
+            if (!(e instanceof AbortException)){


If it is an AbortException, is the message being printed somewhere else already? It needs to be.

The equivalent pattern in the past (of returning false from the perform() method) left no message, so the patch is consistent with that.

Looks also weird for me. Will double-check

The equivalent pattern in the past (of returning false from the perform() method) left no message

Sure, but then the plugin author was deciding to leave no message (or more likely to print one to a BuildListener earlier). If they decided to throw AbortException, they supplied a message, so we should use it.

daniel-beck · 2015-04-29T19:54:13Z

👍

jglick · 2015-04-29T19:54:24Z

test/src/test/java/hudson/model/FreestyleJobPublisherTest.java

+        assertEquals("Build must fail, because we used FalsePublisher", b.getResult(), Result.FAILURE);
+        File file = new File(b.getArtifactsDir(), "result.txt");
+        assertTrue("ArtifactArchiver is executed even prior publisher fails.", file.exists());
+        assertTrue("Second publisher must see FAILURE status", FileUtils.readFileToString(file).equals(Result.FAILURE.toString()));


And I would expect to see the detail message from the AbortException in the log.

Should also be able to

j.assertBuildStatusNotContains("\tat ", b);

to prove that we suppressed the stack trace.

kohsuke · 2015-04-29T20:01:15Z

Tests do pass for me.

jglick · 2015-04-29T20:01:27Z

test/src/test/java/hudson/model/utils/ResultWriterPublisher.java

+    @Override
+    public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
+        FilePath file = build.getWorkspace().child(fileName);
+        file.write(build.getResult().toString(), Charset.defaultCharset().name());


This is fine, though it would be slightly more concise to just print the result in a specially-formatted message to the listener. Then you can use JenkinsRule.assertBuildLogContains.

…though perhaps not, since you seem to be assuming that a following ArtifactArchiver will see it, too.

Interaction with files is not ideal, but it works very well. buildlog can be screwed or has something wrong. File is strict.

jglick · 2015-04-29T20:28:47Z

Reviewed. I would be 👍 if AbortException.message were printed. All else is minor, mostly suggestions for strengthening the tests.

KostyaSha · 2015-04-29T20:30:42Z

Thanks for super review :) I will fix comments

- Print publisher display name instead of class, so user can understand what publisher in UI was used and failed. - Setter for artifact archiver shows how it handle status - Minor typos - Added test for stacktraces in build log

KostyaSha · 2015-04-29T23:57:39Z

core/src/main/java/hudson/model/AbstractBuild.java

                    } catch (LinkageError e) {
                        reportError(bs, e, listener, phase);
+                        r = false;


performAllBuildSteps is ignored in most calls, but i think this should add consistency for failed execution

KostyaSha · 2015-04-30T00:12:38Z

Also rebased and noted 😅
I can wrote more tests but i have no ideas what else can be tested.

jglick · 2015-04-30T13:43:30Z

core/src/main/java/hudson/model/AbstractBuild.java

@@ -723,21 +725,33 @@ protected final boolean performAllBuildSteps(BuildListener listener, Iterable<?
                    try {
                        if (!perform(bs,listener)) {
                            LOGGER.log(Level.FINE, "{0} : {1} failed", new Object[] {AbstractBuild.this, bs});
+                            setResult(Result.FAILURE);


Does this not need to be conditional on phase, like it is in reportError?

Interesting question.

Changed to have consistency.

- Typo - Print error combined - Set result only when phase like in other code parts. - Test for expected error message in log

jglick · 2015-05-01T22:00:44Z

The last PR build failed randomly (timeout after the notorious Credentials-related errors I can never track down), so submitted for validated merge rather than merging immediately.

jglick · 2015-10-05T13:39:03Z

core/src/main/java/hudson/model/AbstractBuild.java

-            String msg = "Publisher " + bs.getClass().getName() + " aborted due to exception";
-            e.printStackTrace(listener.error(msg));
-            LOGGER.log(WARNING, msg, e);
+            final String publisher = ((Publisher) bs).getDescriptor().getDisplayName();


Note, IDE or static analysing tools didn't highlight this code.

KostyaSha changed the title ~~WIP [FIXED JENKINS-26964] Handle AbortException right~~ [FIXED JENKINS-26964] Handle AbortException right Feb 21, 2015

jtnord reviewed Feb 23, 2015
View reviewed changes

KostyaSha force-pushed the JENKINS-26964 branch from a6d989f to 6198db7 Compare February 23, 2015 16:44

orrc reviewed Mar 28, 2015
View reviewed changes

KostyaSha closed this Mar 28, 2015

KostyaSha reopened this Mar 28, 2015

jglick reviewed Apr 29, 2015
View reviewed changes

[FIXED JENKINS-26964] Handle AbortException right

625a22e

KostyaSha force-pushed the JENKINS-26964 branch from 6198db7 to 545e123 Compare April 29, 2015 23:52

Fix PR comments

92734a8

- Print publisher display name instead of class, so user can understand what publisher in UI was used and failed. - Setter for artifact archiver shows how it handle status - Minor typos - Added test for stacktraces in build log

KostyaSha force-pushed the JENKINS-26964 branch from 545e123 to 92734a8 Compare April 29, 2015 23:54

KostyaSha reviewed Apr 29, 2015
View reviewed changes

Note JENKINS-26964

3e856e6

jglick reviewed Apr 30, 2015
View reviewed changes

Fixing PR comments:

161a009

- Typo - Print error combined - Set result only when phase like in other code parts. - Test for expected error message in log

jglick self-assigned this May 1, 2015

jglick merged commit 161a009 into jenkinsci:master May 1, 2015

jglick added a commit that referenced this pull request May 1, 2015

[JENKINS-26964] Merging #1577.

23758d4

jglick reviewed Oct 5, 2015
View reviewed changes

[FIXED JENKINS-26964] Handle AbortException right #1577

[FIXED JENKINS-26964] Handle AbortException right #1577

Conversation

KostyaSha commented Feb 20, 2015

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

KostyaSha commented Feb 23, 2015

ikedam commented Feb 28, 2015

KostyaSha commented Feb 28, 2015

jtnord commented Mar 1, 2015

oleg-nenashev commented Mar 2, 2015

KostyaSha commented Mar 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orrc commented Mar 28, 2015

KostyaSha commented Mar 28, 2015

kohsuke commented Apr 29, 2015

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

daniel-beck commented Apr 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohsuke commented Apr 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Apr 29, 2015

KostyaSha commented Apr 29, 2015

Choose a reason for hiding this comment

KostyaSha commented Apr 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented May 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment