redis / redis/node-redis

Errors from ready check are emitted to client handler instead of being bubbled up to caller.

Open
#1,093 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
17.6k
Forks
2k
Avg merge
2d 3h
Merged PRs (30d)
40

Description

  • Version: 2.6.2
  • Platform: Node 0.12.2 / Windows Server 2012 R2
  • Description: If an error is thrown in ready check, it is emitted through the client's event handler instead of being bubbled up to the calling method.

Example:

  1. Server is busy running a script.
  2. ready check's call for info returns: ReplyError: BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE.
  3. error is emitted via:
err.message = 'Ready check failed: ' + err.message;
this.emit('error', err);
  1. calling method has no chance to handle the error.

Example of how it works with ready check:

var redis = Promise.promisifyAll(require('redis'));
var client = redis.createClient(redisConfig.port, redisConfig.host);

client.on('error', function(err){ //BUSY error ends up here });

function handleError(err) { //BUSY error NEVER gets here };

client.evalAsync(cmd).then(handleReply).error(handleError);

Example of how it works with no_ready_check: true:

var redis = Promise.promisifyAll(require('redis'));
var client = redis.createClient(redisConfig.port, redisConfig.host, {no_ready_check: true});

client.on('error', function(err){ //BUSY error NEVER ends up here });

function handleError(err) { //BUSY error ends up here };

client.evalAsync(cmd).then(handleReply).error(handleError);

Shouldn't the ready check either:

  1. Handle BUSY responses from the server, retrying just like it does when the server is loading data from disk.
    OR
  2. Bubble the error up to the caller instead of through the client.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Trace the client's ready-check entry point and the evalAsync call path described in the issue; no source file or test is named, so locate those entry points first. Reproduce the BUSY response and confirm the chosen behavior: either retry it like loading responses or deliver it to the calling promise rather than the client error handler.

Written by the indexing model from the issue text.

Assessment

Tech stack
node.js, redis, typescript
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.