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

lock_args does not work when you define the lock_args argument and default lock_args function at the same time. #548

Closed
fekadeabdejene opened this issue Nov 2, 2020 · 7 comments · Fixed by #551
Assignees
Milestone

Comments

@fekadeabdejene
Copy link

Version: 7.0.0.beta25

Describe the bug
The self.lock_args class method is never called when the lock_args parameter is defined in the sidekiq_options method.

Expected behavior
The self.lock_args class method should be called when it is defined in the sidekiq_options lock_args: :lock_args. Or the documentation should be updated to reflect this behaviour. Conversely, unique_args works when it is defined in the class and sidekiq_options unique_args: :unique_args. So there should be some consistency there.

Current behavior
Currently, the self.lock_args is never called when it is defined in the lock_args parameter.

Worker class

# this example does not work self.lock_args is never called
class UniqueJobWithFilterMethod
  include Sidekiq::Worker
  sidekiq_options lock: :until_and_while_executing,
                  lock_args: :lock_args 

  def self.lock_args(args)
    [ args[0], args[2][:type] ]
  end

  ...

end
# this example works by not defining the lock_args parameter and using default class function self.lock_args.
class UniqueJobWithFilterMethod
  include Sidekiq::Worker
  sidekiq_options lock: :until_and_while_executing

  def self.lock_args(args)
    [ args[0], args[2][:type] ]
  end

  ...

end

Additional context
Add any other context about the problem here.

@mhenrixon
Copy link
Owner

You must only define the lock_args argument to the sidekiq options if you need to deviate from the default convention. That said, I can see why someone might want to be explicit about it.

I'll see if I can create a failing test for this.

@fekadeabdejene
Copy link
Author

That makes sense. However, I took the failing example copy pasted exactly from the documentation. So it is a little confusing on what the correct definition would be.

@mhenrixon
Copy link
Owner

@fekadeabdejene I can't replicate this in my tests.

Which sidekiq version are you on?

@fekadeabdejene
Copy link
Author

fekadeabdejene commented Nov 2, 2020

6.1.2

@mhenrixon
Copy link
Owner

Great find! This will require some additional changes:

Instead of sidekiq_options lock_args: :lock_args you need to set it to sidekiq_options lock_args_method: :lock_args. Unfortunately, the usage of this key skipped the writing of the arguments completely so every single job where attempted to be set with the unique argument :lock_args.

I won't release it just yet, I need to write some better coverage for this tomorrow. I want to also backport this to v6.

@mhenrixon mhenrixon self-assigned this Nov 2, 2020
@mhenrixon mhenrixon added this to the V7.0 milestone Nov 2, 2020
@mhenrixon
Copy link
Owner

@fekadeabdejene
Copy link
Author

Wow quick work - thank you!

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

Successfully merging a pull request may close this issue.

2 participants