redis-semaphore icon indicating copy to clipboard operation
redis-semaphore copied to clipboard

Connection timeout during refresh throws an unhandled error that shuts down node 16+

Open Farenheith opened this issue 2 years ago • 2 comments

Hello!

I'm using this library to control distributed mutexes on our application and, sometimes, I get an error of connectionTimeout on Redis during the lock refresh. This is an infrastructure problem of course, but the library currently doesn't offer me a way to treat this error. I'm using v5.50 of the lib and using node 20. Here's my error Stack:

Error: Command timed out
at optimizedEval (/app/node_modules/redis-semaphore/lib/utils/createEval.js:24:34)
at refreshMutex (/app/node_modules/redis-semaphore/lib/mutex/refresh.js:26:55)
at RedisMutex._refresh (/app/node_modules/redis-semaphore/lib/RedisMutex.js:31:49)
at RedisMutex._processRefresh (/app/node_modules/redis-semaphore/lib/Lock.js:97:42)
at listOnTimeout (node:internal/timers:573:17)
at process.processTimers (node:internal/timers:514:7)

I think it'd be great if I could inform some listener to treat those unhandled errors during refresh

Farenheith avatar Nov 13 '23 16:11 Farenheith

Hello! Maybe then it would be more convenient to catch this error at the client level, since it is an infrastructure problem? Errors can also occur between refresh calls (e.g. a scheduled server shutdown). https://redis.github.io/ioredis/classes/Redis.html#on

client.on('error', err => { ... })

Handling such errors is out of scope of this lib.

swarthy avatar Nov 15 '23 08:11 swarthy

Hello! Maybe then it would be more convenient to catch this error at the client level, since it is an infrastructure problem?

You're right. But actually ioredis errors are catchable but some parts of the code I'm working on weren't treating errors properly, letting a lot of floating promises unhlandled. I'm spending the week to make it right, and the error described on this thread was just one of the issues I've experienced. But I think I'm almost done with it.

I'll see if I can do a test with nodejs 20 again after I confirm that everything is fine now.

Farenheith avatar Nov 15 '23 09:11 Farenheith

You can disable periodical semaphore refresh with refreshInterval: 0. After awaited .acquire() there is no place where you could catch such errors. If you would prefer something like onRefreshError callback then PR is welcome

swarthy avatar Feb 28 '24 10:02 swarthy