Skip to content

Replace custom semaphores with golang.org/x/semaphore

James Ribe requested to merge jribe/gitlab-runner:build-helper-concurrency into master

What does this MR do?

This MR replaces the custom semaphore logic in builds_helper with the semaphore implementation provided by golang.org/x/semaphore.

This MR renames acquireBuild -> tryAcquireBuild and acquireRequest -> tryAcquireRequest. This mirrors the structure of semaphore.Weighted.

This MR changes releaseBuild and releaseRequest to panic instead of returning false when a bad release is made. As far as I know, none of the calls to those methods are at risk of making bad releases. This mirrors the structure of semaphore.Weighted and sync.Mutex.

To facilitate that change, it also changes the way that buildsHelper indexes its semaphores. The current implementation indexes the semaphores by the runner config's token value and uses the max value provided by the passed runner config. That model was incompatible with semaphore.Weighted because it was effectively creating a semaphore with mutable max.

Both the current implementation and this implementation exhibit some negative behaviors.

The current code:

  • Shares a semaphore among all configs with the same token
  • Uses the max from given config
  • Suffers a collision when two configs use the same token with different concurrency limits
  • Behaves oddly when stale configurations are circulating among the workers.

This new code:

  • Creates a unique semaphore for each config object in memory
  • Uses the max from the original config object
  • Can result in higher-than-specified concurrency values when stale configurations are circulating among the workers.

Why was this MR needed?

Using semaphore.Weighted reduces the amount of synchronization code to be maintained and provides the opportunity to use a blocking call in the future instead of a non-blocking call.

Are there points in the code the reviewer needs to double check?

Does this MR meet the acceptance criteria?

  • Documentation created/updated
  • Tests
    • Added for this feature/bug
    • All builds are passing
  • Branch has no merge conflicts with master (if you do - rebase it please)

What are the relevant issue numbers?

Edited by 🤖 GitLab Bot 🤖

Merge request reports