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

mget with Redis.Cluster and enableAutoPipelining: true causes "Error: All the keys in a pipeline command should belong to the same slot" #1392

Closed
TysonAndre opened this issue Jul 28, 2021 · 3 comments · Fixed by #1391
Labels

Comments

@TysonAndre
Copy link
Collaborator

TysonAndre commented Jul 28, 2021

Discovered while investigating issues similar to ones fixed by #1391

Any command that affects multiple keys is probably also affected (exists/del with more than one key, mset, mget, etc)

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 }], { enableAutoPipelining: true });
    const results = await Promise.all([
      redis.mget(['v{profile}1', 'v{profile}2', 'v{profile}3', 'v{profile}4']),
      redis.get('b'),
    ]);
    console.log(results);
    process.exit(0);
}
main();
(node:35846) UnhandledPromiseRejectionWarning: Error: All the keys in a pipeline command should belong to the same slot
    at Pipeline.exec (/path/to/ioredis/built/pipeline.js:248: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:35846) 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:35846) [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.
^C
@TysonAndre
Copy link
Collaborator Author

Possibly related to #1264 and #1248.

@TysonAndre
Copy link
Collaborator Author

TysonAndre commented Jul 28, 2021

One possible fix would be to check if a command allowed and had multiple keys. The _iterateKeys helper method is called (by getKeys, which is called by lib/pipeline.js to check for pipeline consistency) - maybe a similar check should be added to lib/autoPipelining.ts when client.isCluster in shouldUseAutoPipelining

node_modules/redis-commands/commands.json keyStart != keyStop may be a cheaper to compute heuristic but worse for throughput

EDIT: The real issue I was having was the fact this was attempting to hash an array instead of a string for the mget example (or unlink([key1, key2]), etc)

// lib/command.ts
  /**
   * Iterate through the command arguments that are considered keys.
   *
   * @param {Function} [transform=(key) => key] The transformation that should be applied to
   * each key. The transformations will persist.
   * @returns {string[]} The keys of the command.
   * @memberof Command
   */
  private _iterateKeys(
    transform: Function = (key) => key
  ): Array<string | Buffer> {
    if (typeof this.keys === "undefined") {
      this.keys = [];
      if (commands.exists(this.name)) {
        const keyIndexes = commands.getKeyIndexes(this.name, this.args);
        for (const index of keyIndexes) {
          this.args[index] = transform(this.args[index]);
          this.keys.push(this.args[index] as string | Buffer);
        }
      }
    }
    return this.keys;
  }

TysonAndre added a commit to TysonAndre/ioredis that referenced this issue Jul 28, 2021
Fixes redis#1392

The issue was that this was passing an array instead of a string to
cluster-key-slot when `args[0]` was an array

Handle pathological cases such as redis.get([], ['key1', 'key2'])
by flattening the entire args array.
TysonAndre added a commit to TysonAndre/ioredis that referenced this issue Jul 28, 2021
Fixes redis#1392

The issue was that this was passing an array instead of a string to
cluster-key-slot when `args[0]` was an array

Handle pathological cases such as redis.get([], ['key1', 'key2'])
by flattening the entire args array.
TysonAndre added a commit to TysonAndre/ioredis that referenced this issue Jul 31, 2021
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 redis#1264 and redis#1248.

Fixes redis#1392
TysonAndre added a commit to TysonAndre/ioredis that referenced this issue Jul 31, 2021
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 redis#1264 and redis#1248.

Fixes redis#1392
@luin luin closed this as completed in #1391 Aug 1, 2021
luin pushed a commit that referenced this issue Aug 1, 2021
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

Closes #1264
Closes #1248
Fixes #1392
ioredis-robot pushed a commit that referenced this issue Aug 1, 2021
## [4.27.7](v4.27.6...v4.27.7) (2021-08-01)

### Bug Fixes

* **cluster:** fix autopipeline with keyPrefix or arg array ([#1391](#1391)) ([d7477aa](d7477aa)), closes [#1264](#1264) [#1248](#1248) [#1392](#1392)
@ioredis-robot
Copy link
Collaborator

🎉 This issue has been resolved in version 4.27.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this issue Mar 1, 2024
## [4.27.7](redis/ioredis@v4.27.6...v4.27.7) (2021-08-01)

### Bug Fixes

* **cluster:** fix autopipeline with keyPrefix or arg array ([#1391](redis/ioredis#1391)) ([d7477aa](redis/ioredis@d7477aa)), closes [#1264](redis/ioredis#1264) [#1248](redis/ioredis#1248) [#1392](redis/ioredis#1392)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants