redis-parser referring null values.

Question

redis-parser referring null values.

numabus opened this issue 7 years ago · 5 comments

Hi, redis-parser.

I am developing a solution, and redis is an important part.
I can not trace what the reason is,
On my way to development, I noticed that redis-parser had a problem referring to null values.

Next time you release the patch, I want you to make sure the following variables have null values.

The error message looks like this:

/usr/src/app/node_modules/redis-parser/lib/parser.js:560
while (this.offset < this.buffer.length) {
^

TypeError: Cannot read property 'length' of null
at JavascriptRedisParser.execute (/usr/src/app/node_modules/redis-parser/lib/parser.js:560:35)
at Socket. (/usr/src/app/node_modules/redis/index.js:274:27)
at emitOne (events.js:96:13)
at Socket.emit (events.js:188:7)
at readableAddChunk (_stream_readable.js:176:18)
at Socket.Readable.push (_stream_readable.js:134:10)
at TCP.onread (net.js:547:20)

I'll fix it like this:

  if(this.buffer !== null){
    while (this.offset < this.buffer.length) {
      var offset = this.offset
      var type = this.buffer[this.offset++]
      var response = parseType(this, type)
      if (response === undefined) {
        if (!this.arrayCache.length) {
          this.offset = offset
        }
        return
      }
  
      if (type === 45) {
        this.returnError(response)
      } else {
        this.returnReply(response)
      }
    }
  }

Thank you

Answer 1 · 2017-09-02T05:00:12.000Z

@numabus would you be so kind and tell me what you did and what Node.js version you? And can you activate the debug mode in node_redis as well so I can check what input / output you have? That would be very helpful.

Answer 2 · 2017-09-04T08:20:13.000Z

Thank you BridgeAR.

I read your post and prepare this example.
And meantime I got some hint, that is an error occurs when used with the sleep function.
I want to kick out the sleep function, if I can. Really.
But the sleep function is simple.

I guess that "the exception" fires when trying to parse between unended network handlings.

Here is example project:

OS : osx sierra 10.12.6
Node.js : v6.11.2

package.json

{
  "name": "web_app",
  "version": "1.0.0",
  "description": "Node.js",
  "author": "First Last <first.last@example.com>",
  "main": "server.js",
  "scripts": {
    "start": "node server.js"
  },
  "dependencies": {
    "body-parser": "^1.17.2",
    "cassandra-driver": "^3.2.2",
    "express": "^4.15.4",
    "redis": "^2.8.0",
    "node-etcd": "^5.1.0",
    "system-sleep": "^1.3.5"
  }
}

server.js

const express = require('express');
const bodyParser = require('body-parser');
const redis = require('redis');
var sleep = require('system-sleep');
var redisClient = undefined;

var inProcess = false;

var exampleConfig = {
  name: 'shadow_broker_config',
  contents:[
    { kind: 'redis', name:'redis', host: "192.168.10.66", port: 6379 }
  ]
};

initial(exampleConfig);

// Constants
const PORT = 8080;
const HOST = '0.0.0.0';

// App
const app = express();
app.use(bodyParser.json());

app.get('/', (req, res) => {
  res.send('Hello world\n');
});

function initial( jsonConfig ){
  for(var i=0; i< jsonConfig.contents.length; i++){
    var curConfig = jsonConfig.contents[i];
    console.log('share-redis config:'+JSON.stringify(curConfig));
    if(curConfig.name == 'redis'){
      inProcess = true;
      redisClient = redis.createClient(curConfig.port, curConfig.host, {
        retry_strategy: function (options) {
            if (options.error && options.error.code === 'ECONNREFUSED') {
                return new Error('The server refused the connection');
            }
            if (options.total_retry_time > 1000 * 5) { // 1000 * 60 * 60
                return new Error('Retry time exhausted');
            }
            if (options.attempt > 10) {
                return undefined;
            }
            return Math.min(options.attempt * 100, 3000);
        }
      });
      redisClient.on('error', (err) =>{
        console.log('redis error:'+err);  
      });

      redisClient.on('connect', () => {
        inProcess = false;
        console.log('Redis connected, Successfully!');
      });

      console.log('Redis connect process done, go next.!');
    }
  } // for  
} // function

var inProcess = false;

app.get('/test', (req, res) => {
  redisClient.get("testkey", (err, response) => {
    if(err !== null){
      return res.send({status:'error', msg: err});
    }

    inProcess = true;

    redisClient.set("testkey2", "aaaa",(err, response) => {
      if(err !== null){
        return res.send({status:'error2', msg: err});
      } 

      inProcess = false;
    });

    while(inProcess === true){
      sleep(1); // <<-- it can cause exception case.
    }

    return res.send({status:'ok', msg: response});
  });
});

//-------------------------------------------------------------------------------------------------------
//
// prevent shutting down node.js accidently
//
process.on('uncaughtException', function (err) {
  logger("error", "process.on 'uncaughtException'", 'Caught exception: ' + err);
});

app.listen(PORT, HOST);
console.log(`Running on http://${HOST}:${PORT}`);

debug out

Debugging with legacy protocol because Node.js v6.11.2 was detected.
/usr/local/bin/node --debug-brk=20929 --nolazy server.js 
Debugger listening on [::]:20929
share-redis config:{"kind":"redis","name":"redis","host":"192.168.10.66","port":6379}
Redis connect process done, go next.!
Running on http://0.0.0.0:8080
Redis connected, Successfully!

TypeError: Cannot read property 'length' of null
    at JavascriptRedisParser.execute (/tmp/node-test-parser/node_modules/redis-parser/lib/parser.js:560:35)
    at Socket.<anonymous> (/tmp/node-test-parser/node_modules/redis/index.js:274:27)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at readableAddChunk (_stream_readable.js:176:18)
    at Socket.Readable.push (_stream_readable.js:134:10)
    at TCP.onread (net.js:547:20)

In addition, the above description is somewhat insufficient.
If clause must be replaced with try{} catch cluase.

like this:

  try{
    while (this.offset < this.buffer.length) {
      var offset = this.offset
      var type = this.buffer[this.offset++]
      var response = parseType(this, type)
      if (response === undefined) {
        if (!this.arrayCache.length) {
          this.offset = offset
        }
        return
      }
  
      if (type === 45) {
        this.returnError(response)
      } else {
        this.returnReply(response)
      }
    }
  } catch(e) {
  }

Answer 3 · 2017-09-04T12:51:52.000Z

Please try to minimize the test case further to not include any other libraries and especially by checking what is happening while your loop runs.

In general this is a simple fix for your issue

app.get('/test', (req, res) => {
  redisClient.get("testkey", (err, response) => {
    if(err !== null){
      return res.send({status:'error', msg: err});
    }

    redisClient.set("testkey2", "aaaa",(err, response) => {
      if(err !== null){
        return res.send({status:'error2', msg: err});
      } 

      return res.send({status:'ok', msg: response});
    });
  });
});

Answer 4 · 2017-09-05T04:02:40.000Z

If I can, I would.
You can close this thread.
Thank you.

Answer 5 · 2018-03-09T22:10:29.000Z

@numabus can we reopen this issue? Or can the change I mention below work?

What would be the harm in changing this:
while (this.offset < this.buffer.length) {
to this:
while (this.buffer && (this.offset < this.buffer.length)) {
Would this extra conditional check for the exists of this.buffer just be masking the real issue?