Skip to content

Fix busy loop when "Cancel build on update" is enabled#688

Open
eagletmt wants to merge 1 commit intojenkinsci:masterfrom
eagletmt:fix-busy-loop
Open

Fix busy loop when "Cancel build on update" is enabled#688
eagletmt wants to merge 1 commit intojenkinsci:masterfrom
eagletmt:fix-busy-loop

Conversation

@eagletmt
Copy link
Copy Markdown

@eagletmt eagletmt commented Jul 2, 2018

ghprb enters busy loop with the following steps.

  1. Update pull-request X and start a build X
  2. Update pull-request Y in the same repository and start a build Y
    • Build Y goes to the build queue since build X is currently running
  3. Update pull-request X and start a build X'
    • GhprbCancelBuildsOnUpdate#cancelCurrentBuilds() is called
    • Check if queuedItem is cancellable, but it turns out impossible
      because prId is different
    • project.getQueueItem() is called repeatedly but it doesn't return
      null until build X finishes and build Y starts.
    • At the same time, build X' doesn't go to the build queue because
      it's blocked by GhprbCancelBuildsOnUpdate#onScheduleBuild()

ghprb should use Queue#getItems API to find cancellable builds in the
build queue.

ghprb enters busy loop with the following steps.

1. Update pull-request X and start a build X
2. Update pull-request Y in the same repository and start a build Y
    - Build Y goes to the build queue since build X is currently running
3. Update pull-request X and start a build X'
    - GhprbCancelBuildsOnUpdate#cancelCurrentBuilds() is called
    - Check if queuedItem is cancellable, but it turns out impossible
      because prId is different
    - project.getQueueItem() is called repeatedly but it doesn't return
      null until build X finishes and build Y starts.
    - At the same time, build X' doesn't go to the build queue because
      it's blocked by GhprbCancelBuildsOnUpdate#onScheduleBuild()

ghprb should use Queue#getItems API to find cancellable builds in the
build queue.
@eagletmt
Copy link
Copy Markdown
Author

eagletmt commented Oct 2, 2018

@samrocketman Could you have a look?

Copy link
Copy Markdown
Member

@samrocketman samrocketman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good. Have you had a chance to test this @eagletmt ?

@bjoernhaeuser
Copy link
Copy Markdown
Contributor

@eagletmt this looks like an important fix, though we need your contribution to actually test this. How can this be tested? Were you able to confirm that your patch works?

@bjoernhaeuser
Copy link
Copy Markdown
Contributor

@eagletmt @samrocketman How can we proceed here? :)

@bjoernhaeuser
Copy link
Copy Markdown
Contributor

I have tested this manually locally and the new code works. Though I was not able to reproduce a real bug, this looks to me like an optimization which is just improving the current code.

@eagletmt is this observation correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants