Skip to content
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-46076] Reordering events so WorkflowRun.completed not set until finish cause printed & log closed #131

Merged
merged 8 commits into from Jun 18, 2019

Conversation

Projects
None yet
4 participants
@jglick
Copy link
Member

commented Jun 7, 2019

[JENKINS-46076] Reordering events in WorkflowRun.finish so that compl…
…eted is not set until the finish cause is printed and the log is closed.

@jglick jglick requested review from dwnusbaum and bitwiseman Jun 7, 2019

jglick added some commits Jun 7, 2019

boolean nullListener = false;
synchronized (getLogCopyGuard()) {
nullListener = listener == null;
try {
setResult(r);

This comment has been minimized.

Copy link
@bitwiseman

bitwiseman Jun 7, 2019

Doesn't setResult(r); still need to be guarded?

This comment has been minimized.

Copy link
@jglick

jglick Jun 7, 2019

Author Member

I see no reason why it would.

This comment has been minimized.

Copy link
@bitwiseman

bitwiseman Jun 7, 2019

Other parts of this file have result setting inside the the synch block.

This comment has been minimized.

Copy link
@jglick

jglick Jun 7, 2019

Author Member

Only incidentally I think.

@bitwiseman

This comment has been minimized.

Copy link

commented Jun 7, 2019

// Un-synchronized to prevent deadlocks (combination of run and logCopyGuard)
- last reference to copyGuard (I can't edit it or I'd just do it).

@jglick jglick changed the title [JENKINS-46076] Reordering events in WorkflowRun.finish so that completed is not set until the finish cause is printed and the log is closed [JENKINS-46076] Reordering events so WorkflowRun.completed not set until finish cause printed & log closed Jun 7, 2019

boolean nullListener = false;
synchronized (getLogCopyGuard()) {
nullListener = listener == null;
try {
setResult(r);

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Jun 7, 2019

Member

Once result is set to a non-null value, isBuilding will call isInProgress instead of short-circuiting. Doesn't this mean that the behavior will be the same since FlowExecution.isComplete() == true (or is it false here?), so isInProgress just starts returning false out of the second branch rather than the first due to moving completed around?

A good downstream check might be to remove the workaround here, update workflow-job to the incremental version from this PR, and run pipeline-model-definition's tests a few times to see if we still run into related failures.

This comment has been minimized.

Copy link
@jglick

jglick Jun 7, 2019

Author Member

Doesn't this mean

Unfortunately it does. Actually this could happen regardless of when we call setResult in finish, since this may be called earlier on a truly running build (e.g., by junit). So I will need to do deeper fixes.

remove the workaround here

Thanks for tip, I did not know about this one.

This comment has been minimized.

Copy link
@jglick

jglick Jun 7, 2019

Author Member

I checked the comment in pipeline-model-definition. I believe that situation is unaffected by this patch:

  • waitForCompletion starts waiting
  • result is set to something in the middle of the build, say UNSTABLE
  • FlowEndNode is added to the graph
  • (asynch) GraphListeners start to be notified
  • isBuilding is checked
    • result == null is false, so isInProgress() is called
    • completed is still false
    • exec.isComplete() is true
  • so assertBuildStatus fails
  • now GraphL is notified and finish is called
    • in some order, result is updated, completed is set to true, etc.

Fixing that kind of failure would require a more intrusive patch to isBuilding which is not required for the originally reported class of flakes and which I am not attempting here.

This comment has been minimized.

Copy link
@jglick

jglick Jun 10, 2019

Author Member

…though you could probably do something like

j.waitForCompletion(run);
j.waitForMessage("Finished: FAILURE", run);

which would be reliable after this patch.

Not going to try it so long as jenkinsci/pipeline-model-definition-plugin#325 remains unmerged.

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Jun 10, 2019

Member

Fixing that kind of failure would require a more intrusive patch to isBuilding which is not required for the originally reported class of flakes and which I am not attempting here

Fair enough. My first thought is to add a transient volatile boolean currentlyCompleting field, set it to true before calling setResult() or setting completed = true in finish (set it back to false in a finally block before notifying listeners in finish), and then in isInProgress, if that field is true, return true (if finish can be called multiple times, you'd need something more complicated so a finished build couldn't go back to being in-progress, although it seems unlikely that anyone would be polling isBuilding in that case).

This comment has been minimized.

Copy link
@jglick

jglick Jun 10, 2019

Author Member

Would not work unless you registered a GraphListener.Synchronous which set this field.

Simpler would be to make isBuilding check, say, listener == null like isLogUpdated now does, but I am not confident in such a change. Of course all these methods are only scantily documented in Run, since their original meaning was heavily tied to AbstractBuild semantics.

@jglick jglick removed the work-in-progress label Jun 7, 2019

@jglick

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

Now waiting for INFRA-2140.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Seems to have worked—all downstream builds pass.

@jglick jglick requested a review from dwnusbaum Jun 13, 2019

@dwnusbaum dwnusbaum merged commit d6b2c56 into jenkinsci:master Jun 18, 2019

2 checks passed

continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@jglick jglick deleted the jglick:finish-JENKINS-46076 branch Jun 18, 2019

@Vlatombe

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

Hey @dwnusbaum, would you mind running a release of the plugin? I'd like to integrate the changes in this PR in downstream plugins

@dwnusbaum

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@Vlatombe Ok, I will try to release the plugin today.

@Vlatombe

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

cool, thanks!

@jglick

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2019

I suppose you can just suggest changes in the downstream POMs after it has been released.

@dwnusbaum

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

2.33 was released, I added suggestion to the downstream PRs linked here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.