-
Notifications
You must be signed in to change notification settings - Fork 122
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
Improved WorkflowRun.finish
timing
#357
Improved WorkflowRun.finish
timing
#357
Conversation
@@ -663,7 +664,6 @@ private void finish(@NonNull Result r, @CheckForNull Throwable t) { | |||
onEndBuilding(); | |||
} finally { // Ensure this is ALWAYS removed from FlowExecutionList | |||
FlowExecutionList.get().unregister(new Owner(this)); | |||
listener = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to move up listener
clearing. Otherwise listener
wold be non-null (thus still logUpdated
) at the moment finish
saved, thus failing WorkflowRunTest.basics
.
@@ -118,7 +118,7 @@ public class WorkflowRunRestartTest { | |||
|
|||
@Issue({"JENKINS-45585", "JENKINS-50784"}) // Verifies execution lazy-load | |||
@Test public void lazyLoadExecution() { | |||
story.thenWithHardShutdown(r -> { | |||
story.then(r -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #356 (comment).
This comment was marked as resolved.
This comment was marked as resolved.
…h`, to minimize impact on functional tests
} | ||
|
||
try { | ||
StashManager.maybeClearAll(this, /* or move up before closing getListener()? */new LogTaskListener(LOGGER, Level.FINE)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes a longstanding issue, that for example https://github.com/jenkinsci/artifact-manager-s3-plugin/blob/b8c98c6978ea3d17517e936b73ac4e98b735e934/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java#L355 would not be seen.
WorkflowRun.saveWithoutFailing
WorkflowRun.finish
timing
src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Devin Nusbaum <dwnusbaum@users.noreply.github.com>
I found that
BulkChange
fromCpsFlowExecution.notifyListeners
causedWorkflowRun.completed
to befalse
inbuild.xml
at the momentRunListener.onFinalized
is called. Was noted in 18d78f3 in #27.