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

Concurrency and Throttle cannot both be used on a job #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeremywadsack
Copy link

While the use case for this is likely slim (only one job can be run at a time and no more than 2 in a second or something), there's a possibility that you would want to have both concurrency and throttle on the same job. This would fail with non-proc keys because the ::lock_key method is memoizing the return value when called by the first strategy to acquire a lock, and the second strategy would get the same key.

This resolves that by removing the memoization. That was part of the original design, but in that case each individual strategy memoized it's own lock (e.g. @concurrency_key). In 6fe8aad with refactoring for Rails 5 this changed to a common Base::lock_key method and the memoization became shared.

I don't think that the memoization adds much. The calls that are memoized are a hash lookup, string interpolation, and gsub, none of which are that complex. Additionally, ::lock_key looks like it's only called once per job, so memoization probably has no effect.

This PR includes a test to verify that the keys are different. That seemed a simpler test (and more determinate) that trying to contrive a situation to prove that both locks are working. It also more clearly documents the thing being asserted. Let me know if this works or if you would like more coverage.

@jeremywadsack
Copy link
Author

@nickelser Any chance you could review this? We're using the gem and our fork of it but would love to get a released version that covers this. (I see there are some other PRs that would be useful, like Redis namespace support).

Thanks!

@liang3404814
Copy link

It'd be great if someone can review this!

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