hibiken / asynq

Simple, reliable, and efficient distributed task queue in Go
MIT License
9.65k stars 697 forks source link

[BUG/Discussion] Accessing undeclared keys in LUA scripts #951

Open pior opened 17 hours ago

pior commented 17 hours ago

Describe the bug

I don't know whether this is a bug or not, but there may be an issue with the way this library uses LUA scripts:

Important: to ensure the correct execution of scripts, both in standalone and clustered deployments, all names of keys that a script accesses must be explicitly provided as input key arguments. The script should only access keys whose names are given as input arguments.

Scripts should never access keys with programmatically-generated names or based on the contents of data structures stored in the database.

https://redis.io/docs/latest/commands/eval/

However, many Asynq LUA scripts are doing exactly that, mostly for the task key: https://github.com/hibiken/asynq/blob/3dbda603333da7c47449e3c1fc14f3c681ac58a3/internal/rdb/rdb.go#L227-L238

Stackoverflow says that specifying the keys is important when using Redis Cluster since it ensures that all keys are on the nodes where the lua script is running: https://stackoverflow.com/a/32091333 But this answer also mentions that the behavior is also undefined on a regular Redis 🤔.

There seems to be a few Github issues related to this: https://github.com/hibiken/asynq/issues/480 https://github.com/hibiken/asynq/issues/724 https://github.com/hibiken/asynq/issues/442

Discussion

I'm not sure whether there is a practical solution, but we should at least know and document this situation.

kamikazechaser commented 6 hours ago

Definitely an anti-pattern against redis recommendations. But in most cases these are dynamic keys being passed, not sure how we can go around these. I'll read up more on this.

kamikazechaser commented 6 hours ago

I think we should remove the line on the README and add a caveat about redis cluster compatibility until we confirm ALL scripts are redis cluster compatible (which isn't the case now).