multi modifies input parameter

[jcstanaway]

Using ioredis 3.1.1 / redis 4.0.

When using multi/exec, I build an array of commands and arguments to pass to the multi constructor:

    let pipeline = redis_controller.multi(redisCmds);
    pipeline.exec(function (err, results) { ... }

(I two-step this to more easily unit test and stub out with sinon.)

When the call to multi() returns, redisCmds has been modified. Specifically, the first entry in each array of redisCmds (the "command") is removed.

Here's an example of what is passed into multi():

[
  [
    "set",
    "key1",
    "data1"
  ],
  [
    "zadd",
    "key2",
    1,
    "data2"
  ],
  [
    "zadd",
    "key3",
    1,
    "data3"
  ],
  [
    "zadd",
    "key4",
    1,
    "data4"
  ]
]

Here's what redisCmds looks like after the return from multi():

[
  [
    "key1",
    "data1"
  ],
  [
    "key2",
    1,
    "data2"
  ],
  [
    "key3",
    1,
    "data3"
  ],
  [
    "key4",
    1,
    "data4"
  ]
]

My code later examines the results from exec and tries to use the content of redisCmds to log metrics and details of which commands failed (if any). However, since the 'commands' have been removed from redisCmds, I'm unable to provide the correct logging information.

I'll have to work around this for now by cloning the array, but it does not seem appropriate for multi() to be modifying the caller's data.

[dirkbonhomme]

Nice find! As a general rule I think we should aim to have all input variables be immutable. (as in: ioredis should not modify them in any way)

[luin]

Could you provide a test case for this? The following code:

const Redis = require('ioredis')
const redis = new Redis()

const redisCmds = [
  [
    "set",
    "key1",
    "data1"
  ],
  [
    "zadd",
    "key2",
    1,
    "data2"
  ],
  [
    "zadd",
    "key3",
    1,
    "data3"
  ],
  [
    "zadd",
    "key4",
    1,
    "data4"
  ]
]
const pipeline = redis.multi(redisCmds)
pipeline.exec(function () {})

console.log(redisCmds)

Logs:

[ [ 'set', 'key1', 'data1' ],
  [ 'zadd', 'key2', 1, 'data2' ],
  [ 'zadd', 'key3', 1, 'data3' ],
  [ 'zadd', 'key4', 1, 'data4' ] ]

[jcstanaway]

Interesting. I ran your test code. The only difference was I supplied a password in the Redis constructor. I got:

root@redis:/usr/src/app# node multiTest.js
[ [ 'key1', 'data1' ],
  [ 'key2', 1, 'data2' ],
  [ 'key3', 1, 'data3' ],
  [ 'key4', 1, 'data4' ] ]

The other difference in my production code is that redisCmds was defined using let, not const as the array of commands is built dynamically using redisCmds.push().

Did you run this with 3.1.1? I'm also using node v6.11.1.

Update: Reconfigured Redis to not use a password and re-ran test. Same results.

[jcstanaway]

I dug into the ioredis code. It appears that the issue is in lib/pipline.js -> addBatch(). Specifically at line 185. While this might appear to operate on a local variable, JS uses a mix of pass by reference and pass by value. At line 184, command isn't a copy of comands[i], but rather a reference to commands[i]. The shift at line 185 actually affects the passed in array (commands). I believe this issue could be resolved by changing line 184 to:

var command = commands[i].slice();

command will then actually be a copy.

The shift() method removes the first element from an array and returns that element. This method changes the length of the array.

The slice() method returns a shallow copy of a portion of an array into a new array object selected from begin to end (end not included). The original array will not be modified.

[jcstanaway]

Resolved with v3.1.2.