RFR: 1530: Race with CheckWorkItem in PR bot

[Erik Joelsson]

On Fri, 5 Aug 2022 22:03:04 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> This patch fixes a race in the PR bot. 
>> 
>> Currently, all WorkItems that process PRs get an instance of the PullRequest that was  fetched before the WorkItem instance was created. The data in that PullRequest instance (the parts that aren't dynamically fetched on method calls) may then be stale before the WorkItem even starts running.
>> 
>> With this patch, I'm changing PullRequestWorkItem (the super class of all WorkItems that handle PRs), to only be instantiated with the ID of the PR, and then fetches the actual PullRequest object at the start of the `run()` method. When the `run()` method is called, we are guaranteed to be the only WorkItem currently processing the particular PR, so fetching data there should guarantee a coherent and current view.
>> 
>> In many cases where PullRequestWorkItems were created, the creator first refreshed the PullRequest object to provide an updated view, reflecting any changes made by the creator. These refreshes are no longer necessary, so I've removed them all.
>
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestWorkItem.java line 49:
> 
>> 47:             return true;
>> 48:         }
>> 49:         if (!(prId.equals(otherItem.prId) && bot.repo().isSame(otherItem.bot.repo()))) {
> 
> This means that two work requests can run concurrently if either the `prId` is different or the `bot repo` is different. I presume this is because the `prId` is relative to the repo (which makes sense), right?

Correct. The prId is the number we see in PR links, e.g. 1349 for this one. The implementation of `PullRequest::isSame` is equivalent to the new code. The problem was that I can no longer guarantee to have the PullRequest object available when `concurrentWith` is called (rather, it's guaranteed to not be there), so I had to basically copy the code, and get the `HostedRepository` from the bot instead of the pr. This condition is supposed to be exactly the same as it was pre patch.

-------------

PR: https://git.openjdk.org/skara/pull/1349