Callbacks hang when memcache fails

[FredKSchott]

Fixes #195, and possibly #185

Currently if any memcache server goes down once a connection is made, Memcached requests will hang and not call their callbacks. This comment from the code explains how this is relevant to Jackpot:

// Jackpot handles any pre-connect errors, but does not handle errors
// once a connection has been made, nor does Jackpot handle releasing
// connections if an error occurs post-connect

I've been working with @denisspb on a fix for this. This PR fixes the issue with the hanging callbacks, but since everyone may want to handle this fail differently, it is still up to the consumer to release the connections via close(), end(), etc.

Let me know how I can help get this through. This can be a huge problem in production and should be fixed as soon as possible.

[FredKSchott]

Broken tests are unrelated to this changed, and are occurring on master (see #200)

[3rd-Eden]

@FredKSchott Could you atleast add a test for this fix?

[FredKSchott]

I actually wasn't sure how, the edge case this solves for involves a post-connection error, so the memcached server would need to properly connect via a successful get(), but then the server itself would have to be killed before a second get() to see if the library could handle a server going down.

Do you know how I could test this?

[mattwiller]

It looks like this is blocking us from rolling out a Node.js service to production, since if a memcached server crash causes our frontends to hang we'll be in pretty big trouble. What is the timeline for getting this fix into an npm version?

[FredKSchott]

@3rd-Eden bump! This is still a serious issue, is there anything I can do to help get this looked at / merged?

[3rd-Eden]

@FredKSchott Adding tests.

[FredKSchott]

@3rd-Eden Awesome! I'd actually love some guidance then, since this is a weird case that doesn't currently have any test I want to make sure I do it in a way that works best for you / the project.

As I mentioned above, this fixes an error that occurs when the memcached server crashes. So to test this, we'll need to kill the memcached server. How could you do that from within a test?

[ronkorving]

Bump to all of you :)

[FredKSchott]

@ronkorving @3rd-Eden still interested in fixing, still need help with tests 😐

[FredKSchott]

Actually, this might be a more productive way of putting it: @3rd-Eden, I believe it is a limitation of the testing environment that prevents me from being able to add an automated test for this fix (I would need to shut down the memcached server itself half-way through the test). Am I wrong? And if so, what am I missing?

[FredKSchott]

Bump!

[dylanahsmith]

You could start tcp server on a different port to mock out memcached and connect to that rather than connecting to the actual memcached. That server can just respond to a get with "END\r\n" (cache miss) then do nothing until after the timeout, after which the server can be closed.

[leichter]

We're also looking into using this module but need this issue to be resolved. I was thinking about writing up a test case for this based @dylanahsmith's suggestion, but I'm not sure how to artificially create a socket error in the client from the mock TCP server. I could dig into the client code, find the client's socket, and emit an error event, but that seems too artificial for this test. Any thoughts?

Also, based on my understanding of the source code and how it basically buffers requests on a connection, shouldn't this call the callback for every entry in the metaData array, rather than just the first one? Say I send 5 requests (that all use the same connection), they hang, and then the server dies and the socket receives an error: shouldn't each request get the error?

[atombender]

It's been about 16 months since this was created. As far as I'm concerned, this library isn't usable in production without this fix. Can we get it merged?

[FredKSchott]

I'm no longer using this in production, so I have less bandwidth (and honestly, will power) to devote to it. I'd still love to see this merged, so if anyone wants to carry the torch and figure out those tests please feel free to use my commit / code. /cc @dylanahsmith @leichter

[dylanahsmith]

I'm no longer even using node.js, so am unlikely to pick this up.

[atombender]

@FredKSchott: Can I ask what you are using instead? From the lack of attention to this and other related issues, it feels like this project has been abandoned. However, I've not had any luck in finding a different library to replace it with.

[FredKSchott]

@atombender nothing has replaced it for me, I'm just no longer using memcached.

I do know @mattwiller continued to work with it for at least a part of last year, he may have some ideas for you

[comtaler]

It looks like this repo is not maintained anymore and I can't find a better replacement. I am giving up on memcached. I have switched to use Redis.

[kdybicz]

@FredKSchott you are a Hero! Thank you very much, this bug was really annoying me. I will drop this link here https://github.com/secretescapes/memcached#notice maybe it will turn out to be handy to anyone in need out there ;)