actionhero/node-resque

Don't work methods that use KEYS command(ioredis)

witem opened this issue · 10 comments

witem commented

Hi,
If I set in config keyPrefix. Some commands stop work(it described in ioredis docs: https://github.com/luin/ioredis/blob/master/README.md#transparent-key-prefixing)

Step to reproduce:

  • create NodeResque.Queue with not null options keyPrefix
  • create scheduled job
  • call await queue.timestamps()

Methods that stop working(maybe not all):

  • queue.timestamps()
  • queue.locks()
  • queue.stats()
  • scheduler.checkStuckWorkers()
evantahler commented

Can you confirm that your keyPrefix does not have an special charecters?

As a note this might be conflicting with the node-resque "namespace" option:

witem commented

Yes, it doesn't have any special charecters. And it not conflicting with namespace option.

I use keyPrefix: undefinedshmell

usebaz commented

@evantahler ping =)

evantahler commented

In d3477e4 I've added a number of passing tests which demonstrate that (I think) the ioredis+prefix behavior is working properly.

Please fork this project, and add a failing test to this section which demonstrates the problem you are observing.

witem commented

Hi @evantahler
I add failed tests for queue.timestamps: witem@cad30e1

But I think also broken next methods:

  • queue.locks()
  • queue.stats()
  • scheduler.checkStuckWorkers()
witem commented

Hi @evantahler
It's normal test case?

evantahler commented

Thank you for your examples! That helped me to see that the issue was coming from any command which relies on redis.keys(), as other methods work OK with the keyPrefix.

It seems that supporting a keyPrefix with redis.keys() is explicitly not support by ioredis. It is mentioned in their readme, and in a number of issues:

The most clear response from @luin is

The first argument of KEYS command is a pattern rather than key name, so we don't prefix it with keyPrefix.

and I agree with him. I think that using keyPrefix in this way is dangerous at the command level. That is why we use redis "namespaces" instead in a way we have direct control over, which allows a similar result.

I would recommend either using databases within your redis server to arrange your data, rather thankeyPrefix going forward.

I will add a note to our readme that we do not support keyPrefix with ioredis (#256)

witem commented

@evantahler maybe I can make PR, for allow make connection options namespace an array.

Example:

const connectionDetails = {
  namespace: ['prefix', 'resque'],
}
const connection = new NodeResque.Connection(connectionDetails)

And in method connection.key change unshift(https://github.com/taskrabbit/node-resque/blob/52c11a7932980ebfdcdfd2519d3b508f423680c6/lib/connection.js#L89) to:

if (Array.isArray(this.options.namespace)) {
  args.unshift(...this.options.namespace)
} else {
  args.unshift(this.options.namespace)
}

It's allow to use namespace more flexibly.

evantahler commented

If you make your change non-breaking, ie: it works with both arrays and strings, that could work. Please be sure to test it!

witem commented

Hi @evantahler
I add PR #258