mmkal / handy-redis

A wrapper around node_redis with Promise and TypeScript support.
Apache License 2.0
172 stars 10 forks source link

Catching the error on connection fail is difficult #224

Closed Ekrekr closed 3 years ago

Ekrekr commented 3 years ago

Currently if the TCP connection fails, it won't be caught. For example,

import { createHandyClient, IHandyRedis } from "handy-redis";
const testRedis = createHandyClient({ host: "127.0.0.1", port: 6379 });
try {
    const pingResult = await testRedis.ping();
} catch (e) {
}

won't catch the error:

Error: Redis connection to 127.0.0.1:6379 failed - connect ECONNREFUSED 127.0.0.1:6379
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1128:14)

A workaround for this is to do:

import Redis from "redis";
import { createHandyClient, IHandyRedis } from "handy-redis";
const untypedClient = Redis.createClient({ host: "127.0.0.1", port: 6379 });
untypedClient.on("error", (e: Error) => {
    // This untypedClient error catch prevents uncatchable errors on TCP connection fail.
});
const testRedis = createHandyClient({ host: "127.0.0.1", port: 6379 });
try {
    const pingResult = await testRedis.ping();
} catch (e) {
}

which works fine, but suppresses all errors, which obviously isn't ideal.

mmkal commented 3 years ago

I believe this is due to the underlying redis client. You can reproduce it by creating a client without using this library at all:

const { createClient } = require("redis");

const redis = createClient({ host: "127.0.0.1", port: 9898 }); // 9898 is just a port redis _isn't_ listening on

redis.ping((err, reply) => {
    console.log({ err, reply });
});

👆 this exits with an unhandled ECONNREFUSED error.

Their docs have some suggestions about what to do to handle connection problems: https://www.npmjs.com/package/redis#connection-and-other-events - I'd suggest trying having a look through them. If you think something's missing it probably makes sense to open an issue there. If it isn't fixed there, feel free to comment here or open another issue requesting a workaround. But I don't think it's necessary - your approach of adding a global error handler seems sensible to me. It shouldn't suppress all errors:

const { createHandyClient } = require("handy-redis");

const client = createHandyClient();

client.redis.on("error", err => {
    console.log("global error: " + err);
});

client
    .set("foo", "bar", ["EX", "NotANumberSoShouldResultInAnError"])
    .then(console.log)
    .catch(err => console.log("scoped error:" + err));

👆 prints scoped error: ReplyError: ERR value is not an integer or out of range