Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Sidekiq::Client.default since memoization can break sharding with batches #2505

Merged
merged 1 commit into from
Aug 21, 2015

Conversation

jonhyman
Copy link
Contributor

I think this is a fairly subtle bug. Basically, in our code, the only time Sidekiq::Client.push gets called is from batch callbacks. I think that the memoization of the pool is causing issues. This is really subtle, I believe it has happened to 1 out of thousands of batches for me, but here's a conceptual idea of how it could happen.

# Sets the default client, will be fixed to shard 1
Sidekiq::Client.default

# Change my pool to be shard 2 (perhaps in an initializer)
Sidekiq.redis = shard_2_pool

b = Sidekiq::Batch.new
b.on(:success, DoSomething)
b.jobs { MyWorker.perform_async }

Then the batch callback may run on shard 1 if this worker runs the final job, because the batch callbacks use Sidekiq::Client.push which uses the memoized default. This will then fail with "no such batch" because the batch lives on shard 2.

Basically, default is now dangerous because Sidekiq::Client's pool can be changed.

@mperham
Copy link
Collaborator

mperham commented Aug 21, 2015

The dreadful side effects of trying to avoid an allocation!

mperham added a commit that referenced this pull request Aug 21, 2015
Remove Sidekiq::Client.default since memoization can break sharding with batches
@mperham mperham merged commit 79d672d into master Aug 21, 2015
@jonhyman jonhyman deleted the feature/default branch August 21, 2015 18:33
mperham added a commit that referenced this pull request Aug 24, 2015
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.

2 participants