Pull request locking

bitbucket-server
pullrequest
lock

#1

I’m wondering when should we use com.atlassian.bitbucket.concurrent.LockService#getPullRequestLock ?

From the documentation and source code it’s not too clear under what circumstances we should use this. I assume it’s for when we want to update a pull request where multiple threads could be updating it?

One example of this I may have found is updating a pull request in an asynchronous event handler. For example an event handler registered for the PullRequestOpenedEvent where we add comments, set reviewers or update the PR in any other other way.

Is my assumption of this correct?

What other cases would one use a pull request lock in?


#2

I think the pull request lock is used for clustered instances. If one node/thread needs to make a modification to a pull request object.


#3

Thanks, however the documentation seems a bit misleading on this. It says here:

Locking a pull request in one instance does not lock it in any other.

But I see a ClusterPullRequestLock implementation is returned so what you’ve said makes sense.

Just wondering if for example you have an event handler for an asynchronous event and if multiple event handlers for that event will update the PR (maybe other plugins) then should we also use the pull request lock?

I thought the API may handle this, but I don’t think it does.


#4

@adam.markham,

Locking a pull request in one instance does not lock it in any other.

That takes the documentation a bit out of context. The sentence before that says “Separate instances of this interface are not related.” In other words, if you create a PullRequestLock using getPullRequestLock("a") and create another with getPullRequestLock("b"), locking a PullRequest with the first will not lock it for the second–they are separate instances of locks and may be acquired concurrently.

For an event like PullRequestOpenedEvent, which is raised exactly once for a given PullRequest, and only on a single node in a Data Center installation, and then never raised again, you wouldn’t need locking. There can’t possibly be any concurrency.

If, on the other hand, you were listening for PullRequestRescopedEvent, or some other event that can happen multiple times for the same PullRequest, you may need to lock. It’s up to you to decide whether you need to, though, based on what you’re doing. Should your code be allowed to run concurrently if the event you’re listening to is raised multiple times (potentially on multiple nodes) in a “short” period of time? (Note that, if you’re using PullRequestLock, you should never do so directly on an event thread. Ensure your @EventListener submits the task to an ExecutorService or otherwise moves the processing off the event thread.)

Another possibility would be where your app performs some form of background processing which is allowed to run on multiple nodes concurrently in a Data Center installation. You could use a PullRequestLock to ensure the processors on each node never modify the same PullRequest concurrently if doing so would be problematic.

Where possible, it’s best to design your app so that it doesn’t need cluster-wide locks. However, that’s not always possible.

I thought the API may handle this, but I don’t think it does.

Our API absolutely handles concurrency “correctly” (barring any bugs I don’t know about :slight_smile:). That handling may take the form of concurrent calls working without issue (adding comments, for example), or throwing an exception (calling PullRequestService.update, where concurrent updates would result in nondeterministic final state).

Our API can’t really know the big picture for how you’re using it, though. Only you, the app developer, know whether you want your logic to be able to run concurrently, or whether it might produce unexpected output or other, more subtle issues. PullRequestLock is one tool in your toolbox for controlling concurrency to meet your needs.

Hope this helps,
Bryan Turner
Atlassian Bitbucket