redis / ioredis

🚀 A robust, performance-focused, and full-featured Redis client for Node.js.
MIT License
14.32k stars 1.19k forks source link

Autopipelining custom command executed with "enableOfflineQueue: false" doesn't reject before connection #1511

Closed alex-statsig closed 2 years ago

alex-statsig commented 2 years ago

Example code:

const client = new IORedis({
  enableAutoPipelining: true,
  commandTimeout: 1000,
  connectTimeout: 10000,
  enableOfflineQueue: false,
  maxRetriesPerRequest: 3,
}
client.defineCommand('msum', {
  numberOfKeys: 1,
  lua: "local a={}for b=0,ARGV[2]-1 do a[b+1]=KEYS[1]..ARGV[1]+b*ARGV[3]end;local c=redis.call('mget',unpack(a))local d=0;for e,f in ipairs(c)do if f then d=d+f end end;return d",
});

...

await client.msum();

In this case, the promise for msum never resolves or rejects. For other commands (not custom commands, ex. client.get), an error is thrown "Stream isn't writeable and enableOfflineQueue options is false". For custom commands however, this error is thrown but not connected correctly (I see it as an unhandled error).

If instead I do:

client.msum = () => {
  throw new Error('Not connected yet');
};
this.client.on('connect', () => {
  this.client.defineCommand('msum', {
    numberOfKeys: 1,
    lua: "local a={}for b=0,ARGV[2]-1 do a[b+1]=KEYS[1]..ARGV[1]+b*ARGV[3]end;local c=redis.call('mget',unpack(a))local d=0;for e,f in ipairs(c)do if f then d=d+f end end;return d",
  });
});

Then everything generally seems to work fine, but it doesn't feel like logic I should need to write (would rather be consistent and let ioredis handle the connecting state as it does for other commands).

NOTE: The reason I'm using enableOfflineQueue: false is to ensure commands resolve when the server is down, and to avoid building up a big queue of commands if the server goes down.

luin commented 2 years ago

Hey @alex-statsig 👋

It actually works as expected on my side with ioredis@4.28.5:

const Redis = require("ioredis");

async function main() {
  const redis = new Redis({
    enableAutoPipelining: true,
    commandTimeout: 1000,
    connectTimeout: 10000,
    enableOfflineQueue: false,
    maxRetriesPerRequest: 3,
  });

  redis.defineCommand("msum", {
    numberOfKeys: 1,
    lua: "local a={}for b=0,ARGV[2]-1 do a[b+1]=KEYS[1]..ARGV[1]+b*ARGV[3]end;local c=redis.call('mget',unpack(a))local d=0;for e,f in ipairs(c)do if f then d=d+f end end;return d",
  });

  try {
    await redis.msum("foo");
  } catch (err) {
    console.log("Caught error", err);
  }
}

main();

And I got:

➜  ioredis node test.js
Caught error Error: Stream isn't writeable and enableOfflineQueue options is false
    at Redis.sendCommand (/Users/luin/ioredis-msgpack/node_modules/ioredis/built/redis/index.js:666:24)

Did I miss something?

alex-statsig commented 2 years ago

Hi luin, looks like I was on 4.27.9. After updating to 4.28.5 I can't reproduce the issue anymore either. Sorry about that, thanks!