Skip to content

Commit

Permalink
Fix more edge cases in autopipelining
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
TysonAndre committed Jul 28, 2021
1 parent beefcc1 commit 77ba81a
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 6 deletions.
31 changes: 27 additions & 4 deletions lib/autoPipelining.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import * as PromiseContainer from "./promiseContainer";
import * as commands from "redis-commands";
import * as calculateSlot from "cluster-key-slot";
import asCallback from "standard-as-callback";
import { flatten } from "./utils/lodash";

export const kExec = Symbol("exec");
export const kCallbacks = Symbol("callbacks");
Expand Down Expand Up @@ -60,25 +62,41 @@ function executeAutoPipeline(client, slotKey: string) {
});
}

function commandHasMultipleKeys(
commandName: string,
args: (string | string[])[]
): boolean {
if (!commands.exists(commandName)) {
return false;
}
args = flatten(args);
if (args.length <= 1) {
return false;
}
return commands.getKeyIndexes(commandName, args).length > 1;
}

export function shouldUseAutoPipelining(
client,
functionName: string,
commandName: string
commandName: string,
args: (string | string[])[]
): boolean {
return (
functionName &&
client.options.enableAutoPipelining &&
!client.isPipeline &&
!notAllowedAutoPipelineCommands.includes(commandName) &&
!client.options.autoPipeliningIgnoredCommands.includes(commandName)
!client.options.autoPipeliningIgnoredCommands.includes(commandName) &&
(!client.isCluster || !commandHasMultipleKeys(commandName, args))
);
}

export function executeWithAutoPipelining(
client,
functionName: string,
commandName: string,
args: string[],
args: (string | string[])[],
callback
) {
const CustomPromise = PromiseContainer.get();
Expand All @@ -104,7 +122,12 @@ export function executeWithAutoPipelining(
}

// If we have slot information, we can improve routing by grouping slots served by the same subset of nodes
const slotKey = client.isCluster ? client.slots[calculateSlot(args[0])].join(",") : 'main';
// NOTE: args is the unflattened args.
// ioredis will only flatten arguments by one level in the Command constructor.
const slotKey =
client.isCluster && args.length > 0
? client.slots[calculateSlot(flatten(args)[0])].join(",")
: "main";

if (!client._autoPipelines.has(slotKey)) {
const pipeline = client.pipeline();
Expand Down
4 changes: 2 additions & 2 deletions lib/commander.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ function generateFunction(
}

// No auto pipeline, use regular command sending
if (!shouldUseAutoPipelining(this, functionName, commandName)) {
if (!shouldUseAutoPipelining(this, functionName, commandName, args)) {
return this.sendCommand(
new Command(commandName, args, options, callback)
);
Expand Down Expand Up @@ -228,7 +228,7 @@ function generateScriptingFunction(
}

// No auto pipeline, use regular command sending
if (!shouldUseAutoPipelining(this, functionName, commandName)) {
if (!shouldUseAutoPipelining(this, functionName, commandName, args)) {
return script.execute(this, args, options, callback);
}

Expand Down

0 comments on commit 77ba81a

Please sign in to comment.