Lib assumes first response is for last request (race condition)

[erikmack]

Hello, and thank you for memjs!

To reproduce:

1. Run memcached on remote host across WAN (latency may be required, not sure)
2. In the node shell, run

var memjs = require('memjs'),client = memjs.Client.create();

client.get('hello',function(err,val,extras) {console.log(err + ', ' + val)});client.set('foo','bar',function(err,success) {console.log(err + ', ' + success);});

Assuming key 'hello' does not exist already

Expect: 'Key not found',null // from get request null,true // from set request

Actual:

> client.get('hello',function(err,val,extras) {console.log(err + ', ' + val)});client.set('foo','bar',function(err,success) {console.log(err + ', ' + success);});
undefined
> null, null
MemJS SET: Key not found
Error: MemJS SET: Key not found, null

The 'get' response (with opcode 0 and status 1 = key not found) is executing in the callback for the 'set' request.

In other words, the last callback registered will be executed upon receipt of the first outstanding response.

It may be necessary to correlate requests with response callbacks in some kind of data structure:

callbacks[opcode][key] = onecallback;

Then when a response arrives, the correct callback can be invoked.

I encountered this at the office today, and have reproduced it in a second environment at home.

Let me know if you have any questions, and keep up the good work!

[alevy]

Wow, thanks for finding this @erikmack. This is a pretty serious bug, especially in a server responding to multiple requests concurrently. Looking into it.

[jbreckman]

I think the bug is a bit more egregious (I'm assuming these bugs are related) It looks like if you make a second request before the first request comes back, the response from the first request is applied to both callbacks!

function seeBug()
{
  memcache.get('key1', function(err, result) {
    console.info(result); // will be value for key1  
  });
  memcache.get('key2', function(err, result) {
    console.info(result); // will be value for key1, but should be value for key2!
  });
}

[grimen]

I just got stuck on this when writing an adapter for https://github.com/grimen/node-document - instead had to use the non-SASL driver 'node-memcached' as this bug renders this library useless I'm afraid.

[alevy]

Should be fixed in the master branch. I won't close this bug until I've done some more testing and pushed the new version, though.