-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JENKINS-60354] Add flag to FlowInterruptedException to indicate non-interruptions #51
Conversation
Closing/reopening in the hopes that ci.jenkins.io will see the PR. |
Weird, it looks like everything succeeded, and the incremental build was published, but then the agent got killed before the build completed?
|
src/main/java/org/jenkinsci/plugins/workflow/steps/FlowInterruptedException.java
Outdated
Show resolved
Hide resolved
Rebuilding to try to get an incremental build published. The last build got a 500 internal server error, and I don't see any useful messages in the log. |
The incrementals publisher is hosed, see INFRA-2379, so I deployed a snapshot manually as 2.22-20191211.162318-1. |
See JENKINS-60354.
Downstream PRs:
Problem
This PR and the linked tickets are trying to solve a problem introduced by jenkinsci/pipeline-build-step-plugin#24 that caused failures of the
build
step to be treated like build interruptions by theretry
step, so the build failure was ignored instead of being retried. This happened because thebuild
step started usingFlowInterruptedException
so that it could propagate the exact result of the downstream job instead of always propagatingFAILURE
.FlowInterruptedException
is used for things like manual build interruptions, and so some steps that catch exceptions, such asretry
, have special behavior to rethrow this kind of exception.Fix
This chain of PRs tries to fix the issue by adding a flag to
FlowInterruptedException
that can be set to false to indicate that the exception should not be considered a build interruption, and that this type is only being used to track a specific build result. With this approach, we only need to changebuild
to start setting this flag, and then steps likeretry
to examine this flag when choosing whether to handle or rethrowFlowInterruptedException
.Potential Alternative Fix
I'm not super happy about reusing
FlowInterruptedException
for both interruptions and non-interruptions in this way. An alternative fix could be to introduce a newHasResult
interface with agetResult
method, maybe along with aShowStackTrace
marker interface, haveFlowInterruptedException
implement those interfaces. Then we'd update code that currently only usesFlowInterruptedException
for its result to use theHasResult
interface, and update things that use concrete types to decide whether to show stack traces check for theShowStackTrace
instead. We'd also introduce a newPipelineResultException
that implements those interfaces, and then havebuild
throw that exception instead ofFlowInterruptedException
. That would quite a bit more work though, and would be more complex, requiring changes in workflow-job and workflow-cps in areas involving build completion, so I'm inclined to stick with the simple approach for now.