search

swarthy / redis-semaphore

Distributed mutex and semaphore based on Redis
MIT License
148 stars 28 forks source link

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

Closed Farenheith closed 6 months ago

Farenheith commented 9 months ago

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

swarthy commented 9 months ago

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.

Farenheith commented 9 months ago

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.

swarthy commented 6 months ago

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