NodeRedis / node-redis-parser

A high performance Redis protocol (RESP) parser for JavaScript. Used by Node Redis & ioredis.
MIT License
88 stars 36 forks source link

TypeError: Cannot read property 'length' of null #7

Closed gabrfarina closed 8 years ago

gabrfarina commented 8 years ago

Hi,

I'm using node 6.2.1 on ubuntu 16.04. With the latest release (2.0.2), I'm getting a bunch of errors (happening at random):

  while (this.offset < this.buffer.length) {
                                  ^

TypeError: Cannot read property 'length' of null
    at JavascriptRedisParser.execute (xxx/node_modules/redis-parser/lib/parser.js:396:35)
    at Socket.<anonymous> (xxx/node_modules/redis/index.js:267:27)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at readableAddChunk (_stream_readable.js:172:18)
    at Socket.Readable.push (_stream_readable.js:130:10)
    at TCP.onread (net.js:542:20)

I reckon I don't know how to let you reproduce the problem, but still do you have any clue about what the problem could be? It looks like something in the logic that handles null buffers.

Thanks

BridgeAR commented 8 years ago

Do you have a reproducible test case? And would you be so kind and log your buffer? I likely need the exact output to reproduce this.

gabrfarina commented 8 years ago

It looks like it may actually be a bug (race condition) on our side, and unfortunately it doesn't seem easy to reproduce in other settings, as it is sensitive to the latency of the network and the container it runs into. 😓

Changing this line:

  while (this.offset < this.buffer.length) {

back to:

  let len = this.buffer.length;
  while (this.offset < len) {

almost fixed the race condition for us, or at least we see it far less often. It looks like this.buffer.length gets sometimes overwritten.

A bit of context: we are using deasync to force one of our dependencies to offer a sync API, and that might be the culprit, but we are still digging. We think it may be yielding to the tcp events, and possibly later resuming some code that returns from this.returnReply when it is actually too late and this.buffer.length has been overwritten.

Does any of this make sense to you?

BridgeAR commented 8 years ago

Well the only reason I see why this could fail is that the passed buffer passed from the stream to the parser is already null.