Fix not running callback in pipeline custom command

[phlip9]

Hi!

I just spent the past few hours debugging a problem I had with running a custom lua script in a Pipeline.

Part of the issue is that Pipeline.prototype.sendCommand(...) returns this (a Pipeline instance for chaining) instead of the command's Promise (which every other sendCommand method seems to return).

// lib/pipeline.js L146
Pipeline.prototype.sendCommand = function (command) {
  var position = this._queue.length;

  var _this = this;

  command.promise.then(function (result) {
    _this.fillResult([null, result], position);
  }).catch(function (error) {
    _this.fillResult([error], position);
  });

  this._queue.push(command);

  // seems every where else returns command.promise
  // this is, however, necessary because it's used for chaining
  return this;
};

When this Pipeline instance filters down to line 24 in lib/script.js Script.prototype.execute(...), the condition (result instanceof Promise) fails and any callback the user might have passed in never gets attached to the Promise, and is subsequently never run.

// lib/script.js L14
Script.prototype.execute = function (container, args, options, callback) {
  if (typeof this.numberOfKeys === 'number') {
    args.unshift(this.numberOfKeys);
  }
  if (this.keyPrefix) {
    options.keyPrefix = this.keyPrefix;
  }

  var evalsha = new Command('evalsha', [this.sha].concat(args), options);
  evalsha.isCustomCommand = true;
  var result = container.sendCommand(evalsha);

  // if result is a Promise (how it's returned from most everywhere else), then the
  // callback gets attached and everything works as intended
  if (result instanceof Promise) {
    var _this = this;
    return result.catch(function (err) {
      if (err.toString().indexOf('NOSCRIPT') === -1) {
        throw err;
      }
      return container.sendCommand(new Command('eval', [_this.lua].concat(args), options));
    }).nodeify(callback);
  }

  // result is not a Promise--probably returned from a pipeline chain; however,
  // we still need the callback to fire when the script is evaluated
  // therefore, attach the callback to the Promise
  evalsha.promise.nodeify(callback);

  return result;
};

Without this change, the following test case I added fails:

it('should support callbacks', function(done) {
  var pending = 1;
  redis.pipeline()
    .echo('foo', 'bar', '123', 'abc', function(err, result) {
      // this callback never runs
      pending -= 1;
      expect(err).to.eql(null);
      expect(result).to.eql(['foo', 'bar', '123', 'abc']);
    })
    .exec(function(err, results) {
      expect(err).to.eql(null);
      expect(results).to.eql([
        [null, ['foo', 'bar', '123', 'abc']]
      ]);
      expect(pending).to.eql(0);
      done();
    });
});

I admit to not fully understanding ioredis' entire code structure--hopefully this is the appropriate place to fix this issue :)

Thanks!

[luin]

Awesome! 🍺

[phlip9]

Awesome, thanks!