search

nickelser / activejob-traffic_control

Rate limiting/job enabling for ActiveJob using distributed locks in Redis or Memcached.
301 stars 28 forks source link

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

Open jeremywadsack opened 6 years ago

jeremywadsack commented 6 years ago

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 6fe8aad4 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 commented 5 years ago

@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 commented 4 years ago

It'd be great if someone can review this!