fix(cluster): fix autopipeline with keyPrefix or arg array

[TysonAndre]

Previously the building of pipelines ignored the key prefix.
It was possible that two keys, foo and bar, might be set into
the same pipeline. However, after being prefixed by a configured
"keyPrefix" value, they may no longer belong to the same
pipeline.

This led to the error:
"All keys in the pipeline should belong to the same slots
allocation group"

Similarly, args[0] can be a (possibly empty) array which the Command
constructor would flatten. Properly compute the first flattened key when
autopipelining for a Redis.Cluster instance.

Amended version of https://github.com/luin/ioredis/pull/1335/files - see comments on that PR

This may fix #1264 and #1248.

Fixes #1392

[AVVS]

Keys will hash to different nodes and, therefore, wouldnt allow them to be used inside the same pipeline or am I missing something?

[TysonAndre]

Keys will hash to different nodes and, therefore, wouldnt allow them to be used inside the same pipeline or am I missing something?

I'm not quite sure what you mean. If there are 16385 different keys and 16384 different nodes, at least two of the keys will go to the same node because of https://en.wikipedia.org/wiki/Pigeonhole_principle

The issue is that when a cluster was used, we need to use the prefixed key (prefixed + args[0]) to compute the destination node, not the unprefixed key (args[0])

[TysonAndre]

https://github.com/invertase/cluster-key-slot/blob/62a7bce30c668f83c89f331943e882d2c92899c0/lib/index.js#L147 in this case, there's 16384 possible values from 0 to 0x3FFF and there would be collisions if there were more than 16384 keys, and even with a few hundred keys there are likely to be false positive hash collisions due to https://en.wikipedia.org/wiki/Birthday_problem

EDIT: client.slots[ ... ] then narrows down the keys from 0 to 0x3FFF to an even smaller number of distinct server configurations

[TysonAndre]

An example of the failure seen running this without the patch. With this patch, it succeeds

const IORedis = require('./built');
const process = require('process');
async function main() {
    // Using instructions from https://redis.io/topics/cluster-tutorial
    // redis-cli --cluster create 127.0.0.1:7000 127.0.0.1:7001 127.0.0.1:7002 --cluster-replicas 0
    const redis = new IORedis.Cluster([{ host: '127.0.0.1', port:7000 }], { keyPrefix: 'test:', enableAutoPipelining: true });
    const requests = [];
    for (let i = 0; i < 10; i++) {
        requests.push(redis.get(`k${i}`));
    }
    const results = await Promise.all(requests);
    console.log(results);
    process.exit(0);
}
main();

(node:25595) UnhandledPromiseRejectionWarning: Error: All keys in the pipeline should belong to the same slots allocation group
    at Pipeline.exec (/path/to/ioredis/built/pipeline.js:255:29)
    at Immediate.executeAutoPipeline (/path/to/ioredis/built/autoPipelining.js:35:14)
    at processImmediate (internal/timers.js:463:21)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:25595) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:25595) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

[AVVS]

Suggested change

	const slotKey = client.isCluster
	? client.slots[
	calculateSlot(
	client.options.keyPrefix
	? `${client.options.keyPrefix}${args[0]}`
	: args[0]
	)
	].join(",")
	: "main";
	const prefix = client.options.keyPrefix || '';
	const slotKey = client.isCluster
	? client.slots[
	calculateSlot(`${prefix}${args[0]}`)
	].join(",")
	: "main";

this should likely be cheaper. we can also avoid assigning empty string to keyPrefix if we set it in the constructor

[TysonAndre]

redis.get([], [], ['key']) is technically allowed by ioredis because all of the args get passed to lodash flatten. I added getFirstValueInFlattenedArray and some unit tests to work as a fallback for more common cases.

Less common cases (_.isArguments) may benefit from https://github.com/lodash/lodash/blob/master/.internal/isFlattenable.js but that seems brittle, and I don't know ioredis's policy on using internal implementation details like that

[TysonAndre]

Did something similar, but accounting for the fact that args[0] can be an array

[AVVS]

finally realized this is only related to auto pipelining, change makes sense to me

[TysonAndre]

This is ready for another look

[AVVS]

@luin this LGTM, what do you think?

[TysonAndre]

My changes since the last review are minor:

1. Fixed remaining lint errors, including for prefix.
2. Updated package-lock.json because I'd accidentally used an npm registry mirror globally

[luin]

@luin this LGTM, what do you think?

LGTM! Thanks @TysonAndre

[ioredis-robot]

🎉 This PR is included in version 4.27.7 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀