RedisPool crashing the app: Connection forcefully ended and command aborted. It might have been processed.

[ifokeev]

Don't know why, but I'm catching redis crashes using cubejs, and this exception crashing the app:

https://github.com/cube-js/cube.js/blob/ff20167c83531be1830fdc4b40e5e676cd6791bd/packages/cubejs-query-orchestrator/orchestrator/RedisPool.js#L12

So the idea here to add some error events or somehow process the error

Version: 0.19.50

[paveltiunov]

@ifokeev Hey Ivan! Thanks for posting this! I think code for destroying client isn't quite correct. @jcw- You can be interested in this one as well.

[jcw-]

@paveltiunov interesting, I'll take a look

[jcw-]

Another option would be to call client.quit() here, but I'm not sure that's the right answer either.

Calling client.end(true) will clear the buffers, and if there is a command found in a buffer, it will generate an AbortError. However, I'm not clear where this error is actually being raised/thrown - @ifokeev can you provide the clipped part of the screenshot, at the top? I'm trying to determine what code is actually throwing the exception object created by the redis client so I can better understand how to handle this.

I'm also curious what usage scenario is resulting in this - is this on dev, prod? heavy load? intermittent? on startup or shutdown?

[ifokeev]

@jcw-

can you provide the clipped part of the screenshot, at the top? I don't have

I'm also curious what usage scenario is resulting in this - is this on dev, prod? heavy load? intermittent? on startup or shutdown?

redis crashing on the high load, i don't know why. Cubejs just should handle this state and not crash the full app. try...catch is enough.

[jcw-]

@ifokeev Thanks that helps!

Looking around at what some other libraries do:

This one (sol-redis-pool) does client.end(true) but wraps it in a try/catch: https://github.com/joshuah/sol-redis-pool/blob/291b1a39a8f93982b99d8b55feb5d9edf06fb20f/index.js#L86-L93

This one (node-redis-connection-pool) does client.quit(): https://github.com/silverbucket/node-redis-connection-pool/blob/59715ae927289e73ad778ddc0e87a1f01901d33a/src/redis-connection-pool.js#L111-L115

I'm actually not clear on which one is the most appropriate to use with generic-pool - but as we are already using the former, I think putting a try/catch around it is probably a reasonable path forward.

[github-actions[bot]]

If you are interested in working on this issue, please leave a comment below and we will be happy to assign the issue to you. If this is the first time you are contributing a Pull Request to Cube.js, please check our contribution guidelines. You can also post any questions while contributing in the #contributors channel in the Cube.js Slack.

[ricmatsui]

@ovr It seems like it's related to using the redis driver without an error listener since it states:

"error" client will emit error when encountering an error connecting to the Redis server or when any other in Node Redis occurs. If you use a command without callback and encounter a ReplyError it is going to be emitted to the error listener.

So please attach the error listener to Node Redis.

The unhandled emitted error causes node to exit.

If we use the ioredis driver, it doesn't crash (but we seem to run into other issues with retry), and it mentions:

error emits when an error occurs while connecting. However, ioredis emits all error events silently (only emits when there's at least one listener) so that your application won't crash if you're not listening to the error event.

With this change to the RedisFactory, using redis no longer seems to cause the app to crash. We test this by setting maxclients to a very low value: config set maxclients 5 and making a few concurrent queries:

diff --git a/packages/cubejs-query-orchestrator/src/orchestrator/RedisFactory.ts b/packages/cubejs-query-orchestrator/src/orchestrator/RedisFactory.ts
index ba0fb451..217f5398 100644
--- a/packages/cubejs-query-orchestrator/src/orchestrator/RedisFactory.ts
+++ b/packages/cubejs-query-orchestrator/src/orchestrator/RedisFactory.ts
@@ -50,10 +50,12 @@ export async function createRedisClient(url: string, opts: ClientOpts = {}) {
     options.password = getEnv('redisPassword');
   }
 
-  return decorateRedisClient(
-    redis.createClient({
-      ...options,
-      ...opts,
-    })
-  );
+  const client = redis.createClient({
+    ...options,
+    ...opts,
+  });
+
+  client.on('error', () => client.end(true));
+
+  return decorateRedisClient(client);
 } 

[paveltiunov]

@ricmatsui PR would be much appreciated here!

[rpaik]

👋 a quick reminder that we will be replacing Redis with Cube Store as announced in this blog post.

[igorlukanin]

I believe that this issue is not relevant anymore since Cube Store has replaced Redis for query queue and cache management.

Docs: https://cube.dev/docs/product/deployment#redis

Announcement: https://cube.dev/blog/how-you-win-by-using-cube-store-part-1