Skip to content

Commit

Permalink
fix: Connection instability when using socketTimeout parameter (#1937)
Browse files Browse the repository at this point in the history
The issue occurs when using socketTimeout, causing connections to become
unstable with repeated disconnections and reconnections. This happens due
to incorrect ordering of socket stream event handling.

Changes:
- Use prependListener() instead of on() for `DataHandler` stream data events
- Explicitly call resume() after attaching the `DataHandler` stream listener
- Add tests to verify socket timeout behavior

This ensures the parser receives and processes data before timeout checks,
preventing premature timeouts and connection instability.

Fixes #1919
  • Loading branch information
bobymicroby authored Dec 20, 2024
1 parent af83275 commit ca5e940
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 1 deletion.
5 changes: 4 additions & 1 deletion lib/DataHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,12 @@ export default class DataHandler {
},
});

redis.stream.on("data", (data) => {
// prependListener ensures the parser receives and processes data before socket timeout checks are performed
redis.stream.prependListener("data", (data) => {
parser.execute(data);
});
// prependListener() doesn't enable flowing mode automatically - we need to resume the stream manually
redis.stream.resume();
}

private returnFatalError(err: Error) {
Expand Down
54 changes: 54 additions & 0 deletions test/functional/socketTimeout.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { expect } from 'chai';
import { Done } from 'mocha';
import Redis from '../../lib/Redis';

describe('Redis Connection Socket Timeout', () => {
const SOCKET_TIMEOUT_MS = 500;

it('maintains stable connection with password authentication | https://github.com/redis/ioredis/issues/1919 ', (done) => {
const redis = createRedis({ password: 'password' });
assertNoTimeoutAfterConnection(redis, done);
});

it('maintains stable connection without initial authentication | https://github.com/redis/ioredis/issues/1919', (done) => {
const redis = createRedis();
assertNoTimeoutAfterConnection(redis, done);
});

it('should throw when socket timeout threshold is exceeded', (done) => {
const redis = createRedis()

redis.on('error', (err) => {
expect(err.message).to.eql(`Socket timeout. Expecting data, but didn't receive any in ${SOCKET_TIMEOUT_MS}ms.`);
done();
});

redis.connect(() => {
redis.stream.removeAllListeners('data');
redis.ping();
});
});

function createRedis(options = {}) {
return new Redis({
socketTimeout: SOCKET_TIMEOUT_MS,
lazyConnect: true,
...options
});
}

function assertNoTimeoutAfterConnection(redisInstance: Redis, done: Done) {
let timeoutObj: NodeJS.Timeout;

redisInstance.on('error', (err) => {
clearTimeout(timeoutObj);
done(err.toString());
});

redisInstance.connect(() => {
timeoutObj = setTimeout(() => {
done();
}, SOCKET_TIMEOUT_MS * 2);
});
}
});
25 changes: 25 additions & 0 deletions test/unit/DataHandler.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { expect } from 'chai';
import * as sinon from 'sinon';
import DataHandler from '../../lib/DataHandler';

describe('DataHandler', () => {
it('attaches data handler to stream in correct order | https://github.com/redis/ioredis/issues/1919', () => {

const prependListener = sinon.spy((event: string, handler: Function) => {
expect(event).to.equal('data');
});

const resume = sinon.spy();

new DataHandler({
stream: {
prependListener,
resume
}
} as any, {} as any);

expect(prependListener.calledOnce).to.be.true;
expect(resume.calledOnce).to.be.true;
expect(resume.calledAfter(prependListener)).to.be.true;
});
});

0 comments on commit ca5e940

Please sign in to comment.