memcache-plus icon indicating copy to clipboard operation
memcache-plus copied to clipboard

TypeError: Cannot read property 'type' of undefined

Open kawanet opened this issue 5 years ago • 9 comments

It crashes with the stack trace as below:

node_modules/memcache-plus/lib/connection.js:246
    if (deferred.type === 'autodiscovery') {
                 ^

TypeError: Cannot read property 'type' of undefined
    at Connection.read (node_modules/memcache-plus/lib/connection.js:246:18)
    at Carrier.emit (events.js:315:20)
    at Carrier.EventEmitter.emit (domain.js:486:12)
    at node_modules/carrier/lib/carrier.js:24:12
    at processTicksAndRejections (internal/process/task_queues.js:75:11)

I'm running a benchmark script which calls many get/set/append/remove commands. It works in most cases, and crashes in other cases at the same environment.

  • node v14.15.1
  • memcached (server) 1.5.22
  • memcache-plus (client) 0.2.21

kawanet avatar Dec 08 '20 15:12 kawanet

Connection.prototype.read = function(data) {

    var deferred = this.commandBuffer.first();

    if (deferred.type === 'autodiscovery') { // here

It'd crash when commandBuffer is empty.

kawanet avatar Dec 08 '20 15:12 kawanet

Hi @kawanet ,

Could you please share how I can reproduce this issue. Would like to dig into it.

RomanBurunkov avatar Feb 12 '21 07:02 RomanBurunkov

I'm sorry but I don't have a reproducible code at this moment.

I'm not sure but I guess I've faced the crash when I perform a append method which makes the cached value larger than 1MB after appended. It looks causing an invalid situation around the mecached connection. I guess something of the error handling have failed on server-side or client-side.

Our app has been changed not to make such a larger string appended. I'm now not facing the problem, then.

kawanet avatar Feb 12 '21 08:02 kawanet

We're getting this same issue in a high volume production environment. The app crashes every 5 minutes or so and gets restarted. We had to roll back to a version without memcache for now. In QA environments with a small load, it never happens. Our data items are all generally all very small, but guaranteed under 64KB, so we're nowhere near 1MB. We also do not use append anywhere either, but we constantly see this crash in memcache-plus still.

ryanrubleycoates avatar Apr 14 '22 00:04 ryanrubleycoates

I can reproduce the problem simply by storing an empty string as the value. In Connection.read near the bottom, there is what appears to be a pointless if statement that just breaks memcache-plus in this edge case if (data !== '') {

It appears that if you store an empty string or certain other keywords such as literally the word END as the value, the parser gets out of sync when you try to read that key, and will crash your whole app shortly.

ryanrubleycoates avatar Apr 15 '22 02:04 ryanrubleycoates

This parser might be just fundamentally flawed for the memcached protocol, I'm not sure. It seems to ignore the length value read and stored in metadataTemp and just blindly assume each incoming line after a VALUE response can be a command response instead of part of the data.

ryanrubleycoates avatar Apr 15 '22 02:04 ryanrubleycoates

I fixed this in https://github.com/ryanrubleycoates/memcache-ppp and would be happy if the fixes there (and memcache-pp) made it back to memcache-plus, but I'm not sure if they will. The issue can be caused by return values that are empty strings, keywords such as END, or other things that cause the reply parser to get out of sync.

ryanrubleycoates avatar Apr 19 '22 04:04 ryanrubleycoates

Seeing this issue as well still. I've actually yet to be able to run a successful get using this library because of this on messages larger than 1MB and empty messages.

j-bruce avatar Dec 13 '22 17:12 j-bruce