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

Ensure limit and timeout are always numbers #620

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

pinkahd
Copy link
Contributor

@pinkahd pinkahd commented Jul 1, 2021

This should prevent errors where a string is passed
as the lock_timeout or lock_limit and .positive?
is called.

This should prevent errors where a string is passed
as the `lock_timeout` or `lock_limit` and `.positive?`
is called.
Copy link
Owner

@mhenrixon mhenrixon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how a string can enter there. I'm not super fond of this but if it solves a problem for you I don't mind terribly.

To my knowledge, you'd have to configure like this:

sidekiq_options lock_limit: "2", lock_timeout: "5"

I would like to validate against this and raise a helpful message if that's the case.

@pinkahd
Copy link
Contributor Author

pinkahd commented Jul 1, 2021

I don't understand how a string can enter there. I'm not super fond of this but if it solves a problem for you I don't mind terribly.

To my knowledge, you'd have to configure like this:

sidekiq_options lock_limit: "2", lock_timeout: "5"

I would like to validate against this and raise a helpful message if that's the case.

I'm not 100% sure as well this is our configuration for the job in question 👇🏼 what might happen is the fact that we use Sidekiq 5.2.9 or an older version or Redis 🤔

class SomeWorkerWorker
  sidekiq_options queue: :important, lock: :until_executed, lock_timeout: 1.minute, retry: false
  
  def perform(some_arg_id)
    ...
  end
end

But we're still seeing errors such as:

undefined method `positive?' for "60":String

Screenshot 2021-07-01 at 12 04 54

My guess is that somehow Sidekiq serialises the options as strings instead of numbers 🤔

@mhenrixon mhenrixon merged commit ca3082b into mhenrixon:master Jul 1, 2021
@pinkahd pinkahd deleted the fix-calling-positive-on-a-string branch July 1, 2021 11:27
@mhenrixon
Copy link
Owner

Super weird, thank you for the detailed explanation. I'll cut a new version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants