New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Error, which is returned from retry_strategy remains uncaught #1202
Comments
@pavelsc I tried to reproduce this but so far I could not. Please try to reproduce the issue without any third party modules. Currently you seem to at least use |
I am encountering the same error. If I intentionally provide Redis client a bad URL, the on.error method is not invoked. Here is a simple example:
|
For now I am reverting back to using connect_timeout, which emits on 'error' correctly after a connect timeout expires. |
I'm having the same issue, using a custom retry_strategy with a bad endpoint ends up in "AbortError:" |
This caught me out today as well. From looking at the code briefly, this seems to be intentional behaviour. https://github.com/NodeRedis/node_redis/blob/79558c524ff783000a6027fb159739770f98b10e/index.js#L405 explicitly states that if |
I am having this problem as well, consistently. |
I too am not able to catch errors when receiving an
with:
Debugging the application, i'm getting into the |
I have the same issue, after digging around through the source code I found that if we change And insert this piece of code here (add error to the array right away) if (options.error) {
aggregated_errors.push(options.error);
} It works and emits 'error' correctly. The nested loop in that function does not get executed, because 'command_queue' is empty and error never get's added to the array and thus not emitted. If I understand correctly, it's a quite old piece of code, so we do need input from the maintainers or from @BridgeAR I also saw that on first failed connection emits 'end' event , that might mean something (or not), I picked up Redis two days ago, so not sure how the internals work yet. I'll try to fork and dig around a bit more when if/when I have the time. |
And literally the next issue seems connected to this problem #1198 |
@v1adko I'm currently traveling, but I'll try to take a look at it later today or tomorrow (unless Ruben beats me to it). |
I have my redis url deliberately wrong to test error scenarios but i see my retry_strategy not invoked when trying to connect to redis. retry_strategy is invoked only when connection is closed. `const redis = require('redis'); module.exports.connect = () => {
} const qualifyUrl = (url) => { ` Could someone please help me resolve this issue. |
Same here. This nasty hack seems to create the expected behaviour, but not sure if it has any wider implications: const client = redis.createClient({
retry_strategy: ({error}) => client.emit('error', error)
});
client.on('error', console.error); |
I'm having the same issues at the moment. Using retry_strategy, returning the error as indicated by the example in the Readme, yet no error is emitted by the client. The fixes proposed by @v1adko solve the issue at least at face value. I'm wondering what the backwards incompatibility mentioned here is? As pointed out by @maael , the behaviour appears to be intentional for when retry_strategy is set. So is the behaviour expected, but the documentation is incorrect? Should I be emitting errors for the client manually as suggested by @c24w ? edit: As I'm digging into the package I'm realising that emitting manually is probably not the way forward. Seems I need to understand the breaking changes mentioned. |
Any news ? I have the same problem. |
Any news? |
is a wrong idea to do: if (options.error && options.error.code === 'ECONNREFUSED') {
// End reconnecting on a specific error and flush all commands with
// a individual error
return Math.min(options.attempt * 100, 3000);
}``` |
having the same issue, retry_Strategy not triggering error event, no fix yet? |
Did anyone succeed? |
We switched our implementation to https://github.com/luin/ioredis instead, which brought a few improvements (native Promises, lazyConnect(avoid opening a connection when instanciating the redis client, helped us handle errors exactly where we needed)), and allows the following code to run: let cachedItem;
try {
logger.debug(`Fetching GraphCMS query in redis cache...`);
// XXX If fetching data from redis fails, we will fall back to running the query against GraphCMS API in order to ensure the client gets the data anyway
cachedItem = await redisClient.get(body);
} catch (e) {
logger.debug(`An exception occurred while fetching redis cache.`);
logger.error(e);
epsagon.setError(e);
} Using the following import { createLogger } from '@unly/utils-simple-logger';
import Redis from 'ioredis';
import epsagon from './epsagon';
const logger = createLogger({
label: 'Redis client',
});
/**
* Creates a redis client
*
* @param url Url of the redis client, must contain the port number and be of the form "localhost:6379"
* @param password Password of the redis client
* @param maxRetriesPerRequest By default, all pending commands will be flushed with an error every 20 retry attempts.
* That makes sure commands won't wait forever when the connection is down.
* Set to null to disable this behavior, and every command will wait forever until the connection is alive again.
* @return {Redis}
*/
export const getClient = (url = process.env.REDIS_URL, password = process.env.REDIS_PASSWORD, maxRetriesPerRequest = 20) => {
const client = new Redis(`redis://${url}`, {
password,
showFriendlyErrorStack: true, // See https://github.com/luin/ioredis#error-handling
lazyConnect: true, // XXX Don't attempt to connect when initializing the client, in order to properly handle connection failure on a use-case basis
maxRetriesPerRequest,
});
client.on('connect', function () {
logger.info('Connected to redis instance');
});
client.on('ready', function () {
logger.info('Redis instance is ready (data loaded from disk)');
});
// Handles redis connection temporarily going down without app crashing
// If an error is handled here, then redis will attempt to retry the request based on maxRetriesPerRequest
client.on('error', function (e) {
logger.error(`Error connecting to redis: "${e}"`);
epsagon.setError(e);
});
return client;
}; And import { getClient } from './redis';
let redisClient;
let redisClientFailure;
describe('utils/redis.js', () => {
beforeAll(() => {
redisClient = getClient();
redisClientFailure = getClient('localhost:5555', null, 0); // XXX This shouldn't throw an error because we're using lazyConnect:true which doesn't automatically connect to redis
});
afterAll(async () => {
await redisClient.quit();
await redisClientFailure.quit();
});
describe('should successfully init the redis client', () => {
test('when provided connection info are correct', async () => {
// Environment variables are from the .env.test file - This tests a localhost connection only
expect(redisClient.options.host).toEqual(process.env.REDIS_URL.split(':')[0]);
expect(redisClient.options.port).toEqual(parseInt(process.env.REDIS_URL.split(':')[1], 10));
expect(redisClient.options.password).toEqual(process.env.REDIS_PASSWORD);
});
test('when connection info are incorrect', async () => {
expect(redisClientFailure.options.host).toEqual('localhost');
expect(redisClientFailure.options.port).toEqual(5555);
});
});
describe('should successfully perform native operations (read/write/delete/update)', () => {
test('when using async/await (using native node.js promises)', async () => {
const setResult = await redisClient.set('key-1', 'value-1');
expect(setResult).toEqual('OK');
const result = await redisClient.get('key-1');
expect(result).toEqual('value-1');
const delResult = await redisClient.del('key-1');
expect(delResult).toEqual(1);
const setResultB = await redisClient.set('key-1', 'value-1b');
expect(setResultB).toEqual('OK');
const resultB = await redisClient.get('key-1');
expect(resultB).toEqual('value-1b');
const setResultC = await redisClient.set('key-1', 'value-1c');
expect(setResultC).toEqual('OK');
const resultC = await redisClient.get('key-1');
expect(resultC).toEqual('value-1c');
});
});
describe('should allow to catch an error when failing to open a connection to redis, in order to gracefully handle the error instead of crashing the app', () => {
test('when connection info are incorrect', async () => {
expect(redisClientFailure.options.host).toEqual('localhost');
expect(redisClientFailure.options.port).toEqual(5555);
try {
await redisClientFailure.set('key-1', 'value-1'); // This should throw an error, because the connection to redis will be made when executing the
expect(true).toBe(false); // This shouldn't be called, or the test will fail
} catch (e) {
expect(e).toBeDefined();
expect(e.message).toContain('Reached the max retries per request limit');
}
await redisClientFailure.quit();
});
});
}); Env variables: REDIS_URL=localhost:6379
REDIS_PASSWORD=mypasswordissostrong |
I'm also encountering this issue. Will try switching to https://github.com/luin/ioredis for now |
Same issue. Here is where an error is being thrown before retrying.
|
same issue here, do you have a fix using this package? because, I have a solution using IO Redis, but i wanted to use nests-redis directly. |
#1202 (comment) was my fix, basically using a different library. |
I have the code below and try to imitate dropped connection using
iptables -A OUTPUT -p tcp --dport 6379 -j REJECT
.It is supposed that all redis errors are emitted in error event. But here is what i've got in console output:
And as a question: Why it takes about 10 minutes to detect that connection is gone? Is there any way to raise an error in case of no response within 10 seconds? May be any option like response_timeout etc.
The text was updated successfully, but these errors were encountered: