Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeError: Cannot read property 'type' of undefined #75

Open
kawanet opened this issue Dec 8, 2020 · 9 comments
Open

TypeError: Cannot read property 'type' of undefined #75

kawanet opened this issue Dec 8, 2020 · 9 comments

Comments

@kawanet
Copy link

kawanet commented Dec 8, 2020

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
Copy link
Author

kawanet commented Dec 8, 2020

Connection.prototype.read = function(data) {

    var deferred = this.commandBuffer.first();

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

It'd crash when commandBuffer is empty.

@RomanBurunkov
Copy link

Hi @kawanet ,

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

@kawanet
Copy link
Author

kawanet commented Feb 12, 2021

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.

@ryanrubleycoates
Copy link

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
Copy link

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
Copy link

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
Copy link

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.

@j-bruce
Copy link

j-bruce commented Dec 13, 2022

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.

@KAJed82
Copy link

KAJed82 commented May 16, 2024

This still occurs on some data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants