Skip to content

Add Sidekiq 6.1 support

Context

Adding support for Sidekiq 6.0 is a piece of cake as there were no changes that affect the gitlab-sidekiq-fetcher.

However, version 6.1 introduced a breaking change. See merge request here

TL;DR;

There is a :fetch option what allows to configure a custom fetcher. It's a class that should implement an interface that Sideqik is using to fetching jobs from Redis. In < 6.1 the :fetch option was expecting a class reference and sidekiq was initializing the class itself. Since 6.1 the fetch option must be an instance of such a fetcher class.

Initially, it seemed to be easy: just changing class methods to instance methods. That did the job and integration tests started to be green.

I also wanted this gem to still support older versions of sidekiq (5 < x < 7). I first run the gem with sidekiq 6.0 to check if all works and it worked well. Then I run with sidekiq 6.1 and I started to observe the following error in Sidekiq logs:

FiberError: fiber called across threads Enumerator next

and the traceback was showing this line.

I send a good amount of time trying to figure out why integration tests don't throw the same error. I was thinking that the app I'm testing has a different setup or initializes the threads differently. Only after deep debugging, I realized that the error only appears when there are multiple queues.

After changing integration tests to run multiple queues they guided me to the root cause.

ReliableFetch strategy issue

For the ReliableFetch fetcher we use Enumerator instance to iterate through the queues. Enumerator is created in during class initialization. It is fine for Sidekiq < 6.1 because the fetch class is initialized inside every Thread and is owned and used only by this Thread.

In 6.1 initialization is done once before creating threads and one instance is passed to all the Threads.
We can't use Enumerator in such a case as it is not thread-safe. First I've tried using Queue (as this is advertised as thread-safe replacement for Enumerator but it won't help here because it is cross-thread and as a result, all threads have access to the same iterator. Also, w need an infinite (cycle) iterator and it's hard to achieve with Queue.

With @vsizov we decided to go simple and get rid of the Enumerator and instead shuffle the array of queues inside every retrive_unit_of_work call (for not-ordered queues). It should be fine for avoiding queues starvation. (This is also used by default sideqik fetcher and SemiRiableFetch. (Actually the Enumerator wasn't essential and intentional)

Sidekiq version support

This MR releases new version of the gem 0.7.0 and since this version only Sidekiq >=6.1 will be supported. If someone would like to use it with older sidekiq we recommend to use version 0.5 of this gem. This is because of the braking changes described above.

Changes

  • Added support for sidekiq >= 6.1 by replacing class methods with instance ones and setup method
  • Dropped support for sidekiq < 6.1 and added a note about that to the README
  • Changed the implementation of retrive_unit_of_work in ReliableFetch strategy as described above to by replacing Enumerator with an Array (shuffled or ordered) in ReliableFetch

In tests

  • Changed integration tests and spec to run against Sidekiq ~> 6.1
  • Made integration tests running workers with multiple queues (There is a lot of code that is operating on multiple queues and in real life, you use multiple queues and I thought that it would be good for the integration tests to cover that - it also helped me to catch a bug with FiberError)
  • Simplified how we get job id (jid) in ReliabilityTestWorker
Edited by Adam Mazur

Merge request reports

Loading