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 no-op code, protect global space from test code #86

Merged
merged 1 commit into from
Jun 22, 2015

Conversation

stevenjonescgm
Copy link
Contributor

I'm working on a different PR (that's potentially contentious) and found some accidentally-working code.

Maybe middleware/sever/unique_jobs.rb#call was intended to release the lock if

  1. an exception was raised
  2. before_yield? was true but redis failed to delete the key.

However,

(unlocked != 1) will always be true because (without exceptions) unlocked will either be integer 0 or string "1" or maybe string "\"1\"" because of the call to .inspect()

This code happened to to leave the lock in place if option['unique_unlock_order'] == :never because !defined? unlocked || unlocked != 1 is the same as !defined?(unlocked || unlocked != 1) not !defined?(unlocked) || (unlocked != 1)

I spent some time thinking about how to write a test case to demonstrate, but I think that may not be possible since even if I stub one of the methods before the unlocked assignment to raise, unlocked is still a defined local_variable.

If the design was to unlock on exception, then that logic ought to go into after_yield? with a side-effect from unlock(payload_hash) and maybe add a never_unlock? or always_unlock? method.

as for unique_jobs_spec.rb, "when get_sidekiq_options[:unique_unlock_order] is nil" has an order-dependency on being run before any .sidekiq_options unique_unlock_order: :before_yield etc so I made the tests clone UniqueWorker before modifying class_variables

@mhenrixon mhenrixon merged commit 1b2d441 into mhenrixon:master Jun 22, 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