Fix TypeError: Cannot call method 'toString' of null

[devongovett]

It should emit an error rather than throw an uncaught exception. From our logs:

uncaughtException : TypeError: Cannot call method 'toString' of null
at RedisClient.on_info_cmd (/data/storify/api/node_modules/redis/index.js:320:17)
at /data/storify/api/node_modules/redis/index.js:365:14
at try_callback (/data/storify/api/node_modules/redis/index.js:520:9)
at RedisClient.return_reply (/data/storify/api/node_modules/redis/index.js:590:13)
at ReplyParser.<anonymous> (/data/storify/api/node_modules/redis/index.js:263:14)
at EventEmitter.emit (events.js:95:17)
at ReplyParser.send_reply (/data/storify/api/node_modules/redis/lib/parser/javascript.js:297:10)
at ReplyParser.execute (/data/storify/api/node_modules/redis/lib/parser/javascript.js:198:22)
at RedisClient.on_data (/data/storify/api/node_modules/redis/index.js:476:27)
at Socket.<anonymous> (/data/storify/api/node_modules/redis/index.js:79:14)
at EventEmitter.emit (events.js:95:17)
at Socket.<anonymous> (_stream_readable.js:746:14)
at EventEmitter.emit (events.js:92:17)
at emitReadable_ (_stream_readable.js:408:10)
at emitReadable (_stream_readable.js:404:5)
at readableAddChunk (_stream_readable.js:165:9)
at Readable.push (_stream_readable.js:127:10)
at onread (net.js:526:21)

[Philmod]

+1

[mranney]

That looks reasonable to me. @brycebaril, your thoughts?

[devongovett]

Any updates on this? Thanks!

[brycebaril]

If there is no err won't err.message throw a TypeError here?

[brycebaril]

Help me understand what this fixes and why the fix works... (see my comment on the commit)

[devongovett]

Good catch on that. Fixed.

As for what it fixes, we were seeing logs like the one above where res was null for some reason, and therefore toString couldn't be called. At the same time, there was no error, so this caused an uncaught exception and crashed our servers. I'm not familiar enough with the node-redis code to say why res was null, but either way, emitting errors is much better than causing uncaught exceptions IMO, since they can be much more gracefully handled.

[xdamman]

Any update on this?

[cstigler]

+1 plea for merging, this crashes our node process every few weeks

[brycebaril]

The reasons I have not merged this yet are there are no tests or a reproduction case, but primarily, I don't believe this is actually a workable solution to this problem.

The server is replying with no error but a null response to the INFO command. By default this means we should not emit an error, because there is none.

What's most likely happening here is a redis-as-a-service is being used, and it is via-configuration or via-proxy returning null to the info command. If so, this is likely intentional and causing it to fail the ready check will probably cause issues. In this case we should likely just allow the ready check to pass if there is no error but the info content is null.

If there is another reason that the info command is replying with null there are other problems and we should look into why that is happening vs. just handling this error case. Even though we may learn that all we can do is emit an error, we should know why.

This is obviously impacting multiple people so something should be done soon, but until we know the correct path to take because we know why it is happening, I'm not eager to commit to either direction.

[BridgeAR]

@devongovett How does hiredis behave here?

[BridgeAR]

@devongovett @cstigler @xdamman @Philmod I'd like to fix this issue in 2.1. Could anyone of you give me any insights on the redis server and check what it returns when running a info command against it?

[cstigler]

@BridgeAR this has not crashed our server since December 2014. This is weird, since it was crashing every few weeks for several months before that. Not sure what the cause for the improvement was.

[devongovett]

Same, haven't seen this issue in a long time. But IMHO this kind of defensive programming is still a good idea to prevent crashes due to unknown responses from redis.

[BridgeAR]

That's strange. Was there any version update in the mean time? I guess not?