observed HAProxy/Redis/Sentinel error on short timeouts or disconnect

[andrewwolverton]

When HAProxy timeout is set to a very short value, or when connecting directly to Redis and Redis dies, observed the following exception:

Error: Callback was already called.
at /usr/src/thing-proxy/node_modules/ponte/node_modules/async/lib/async.js:43:36
at /usr/src/thing-proxy/node_modules/ponte/lib/ponte.js:42:7
at /usr/src/thing-proxy/node_modules/ponte/node_modules/mosca/lib/persistence/redis.js:161:11
at /usr/src/thing-proxy/node_modules/ponte/node_modules/async/lib/async.js:52:16
at Object.async.forEachOf.async.eachOf (/usr/src/thing-proxy/node_modules/ponte/node_modules/async/lib/async.js:236:30)
at Object.async.forEach.async.each (/usr/src/thing-proxy/node_modules/ponte/node_modules/async/lib/async.js:209:22)
[...]

[mcollina]

There is a failing test on the Redis persistence:

https://travis-ci.org/mcollina/mosca/jobs/136546187#L1059-L1075.

https://github.com/mcollina/mosca/blob/master/test/persistence/abstract.js#L336-L351

[andrewwolverton]

@mcollina This version appears to be failing tests regardless of the minor change I've made here. If i revert those changes, the tests still fail for me. Thoughts?

[mcollina]

how does that fails? with the same reason?

[andrewwolverton]

Yes, unfortunately.

1. mosca.persistence.Redis subscriptions should load the offline client subscriptions:
   Uncaught AssertionError: expected undefined to deeply equal {}
   at test/persistence/abstract.js:348:28
   at Multi.callback (lib/persistence/redis.js:343:7)
   at multi_callback (node_modules/redis/lib/multi.js:88:14)
   at Command.callback (node_modules/redis/lib/multi.js:115:9)
   at normal_reply (node_modules/redis/index.js:714:21)
   at RedisClient.return_reply (node_modules/redis/index.js:816:9)
   at JavascriptRedisParser.Parser.returnReply (node_modules/redis/index.js:188:18)
   at JavascriptRedisParser.execute (node_modules/redis/node_modules/redis-parser/lib/parser.js:408:12)
   at Socket. (node_modules/redis/index.js:267:27)
   at readableAddChunk (_stream_readable.js:153:18)
   at Socket.Readable.push (_stream_readable.js:111:10)
   at TCP.onread (net.js:531:20)

[andrewwolverton]

BTW... the bit in the test file

      this.instance.lookupSubscriptions(client, function(err, results) {
        expect(results).to.eql({});
        done();
      });

During the failure results = undefined

[mcollina]

Then probably it's related to one of the dep changed. Can you update the PR to handle that?

[mcollina]

is err set?

[andrewwolverton]

Nope. No err.

err: null, results: undefined

[mcollina]

results can't be null because of https://github.com/mcollina/mosca/blob/master/lib/persistence/abstract.js#L103-L109.

Can you please have a look and fix that as well?

[andrewwolverton]

The oddness here is that https://github.com/mcollina/mosca/blob/master/lib/persistence/redis.js#L338-L344 the multi.get doesn't seem to be executing, thus resulting in a non err with an undefined response. Any clue why this would be happening?

Has all of this redis code been reworked with the pull requests that use ioredis.js instead of redis.js?

And if I explicitly check for !responses in the cb(err, responses) call, subsequent tests involving the presence of objects like:

      + expected - actual

      -{}
      +{
      +  "hello": {
      +    "qos": 1
      +  }
      +}

... fail.

It seems to be that something fundamental the tests has recently changed and caused these failures.

[mcollina]

something changed in the dependency tree, not in Mosca or Ascoltatori :(.

That PR will be part of v2, because it's switching to ioredis, and that needs some time for me to solve issues there. You can definitely help (we need to revert a PR that was using a lua script, see #464).