Update patterns with fix for v4 #2208

bakkerthehacker · 2021-11-08T17:40:29Z

Ensure opts are passed into redis in the example code for reusing redis connections.

Ensure opts are always passed into redis, allowing for enableReadyCheck and maxRetriesPerRequest to be updated correctly for v4

manast · 2021-11-12T04:45:33Z

Hmm, but with your change there is no re-use of the redis instances since you are creating new ones all the time.

bakkerthehacker · 2021-11-12T07:17:24Z

Hmm, but with your change there is no re-use of the redis instances since you are creating new ones all the time.

Not sure what you mean, there is still re-use. The redis instances are still cached, they are only created once and only if they have not been created yet.

You can try it yourself, it works as expected:

> opts.createClient('client') === opts.createClient('client')
true
> opts.createClient('subscriber') === opts.createClient('subscriber')
true
> opts.createClient('bclient') === opts.createClient('bclient')
false
>

manast · 2021-11-12T07:41:27Z

Reuse in this context means reuse between different Queues, within the same Queue you do not need to do anything special...

bakkerthehacker · 2021-11-12T08:22:26Z

Reuse in this context means reuse between different Queues

Redis is indeed reused between different Queues in this case. This can be verified with some simple logging:

const opts = {
  // redisOpts here will contain at least a property of connectionName which will identify the queue based on its name
  createClient: function (type, redisOpts) {
    switch (type) {
      case 'client':
        if (!client) {
          console.log('new client')
          client = new Redis(REDIS_URL, redisOpts);
        }
        return client;
      case 'subscriber':
        if (!subscriber) {
          console.log('new subscriber')
          subscriber = new Redis(REDIS_URL, redisOpts);
        }
        return subscriber;
      case 'bclient':
        return new Redis(REDIS_URL, redisOpts);
      default:
        throw new Error('Unexpected connection type: ', type);
    }
  }
}

> const queueFoo = new Queue('foobar', opts);
new client
undefined
> const queueQux = new Queue('quxbaz', opts);
undefined
> queueFoo.process(() => '')
Promise { <pending> }
> new subscriber
> queueQux.process(() => '')
Promise { <pending> }

Or by spying on redis:

const sinon = require('sinon');
const Redis = require('ioredis');
const SpyRedis = sinon.spy(Redis);
let client;
let subscriber;

const opts = {
  // redisOpts here will contain at least a property of connectionName which will identify the queue based on its name
  createClient: function (type, redisOpts) {
    switch (type) {
      case 'client':
        if (!client) {
          client = new SpyRedis(REDIS_URL, redisOpts);
        }
        return client;
      case 'subscriber':
        if (!subscriber) {
          subscriber = new SpyRedis(REDIS_URL, redisOpts);
        }
        return subscriber;
      case 'bclient':
        return new Redis(REDIS_URL, redisOpts);
      default:
        throw new Error('Unexpected connection type: ', type);
    }
  }
}

> 
> SpyRedis.getCalls().map(({args}) => args)
[
  [
    undefined,
    {
      maxRetriesPerRequest: null,
      enableReadyCheck: false,
      port: 6379,
      host: '127.0.0.1',
      db: undefined,
      retryStrategy: [Function: retryStrategy],
      connectionName: 'bull:Zm9vYmFy'
    }
  ],
  [
    undefined,
    {
      maxRetriesPerRequest: null,
      enableReadyCheck: false,
      port: 6379,
      host: '127.0.0.1',
      db: undefined,
      retryStrategy: [Function: retryStrategy],
      connectionName: 'bull:Zm9vYmFy'
    }
  ]
]
>

manast · 2021-11-12T08:41:21Z

And what is your explanation for this?

bakkerthehacker · 2021-11-12T14:41:08Z

Didn't really think such simple debugging required an explanation, allow me:

There are many things a developer might do to determine whether a piece of code is being run or called. Temporarily adding in a few console.log() lines to some code allows a developer to determine what code is being run. In this example, I added the log lines console.log 8000 ('new client') and console.log('new subscriber') to the respective client and subscriber sections of createClient. When these log lines are printed, it is understood that the code containing the log line is being run. If the log lines are not printed, it is also understood the code containing the log is not being run. By creating and calling functions on multiple queues, the log lines can be inspected to determine what is being run. Creating the first queue shows the log line new client, indicating that a new client has been created for this first Queue. Creating more queues after this does not show the log line of new client. This indicates that for subsequent queue creation, no new client Redis connection is created. The code with the log line and the Redis constructor are not run. Furthermore, calling a function such as .process() on these queues for the first time shows the log line new subscriber. Calling this function again on a different queue does not show the log new subscriber. Similarly to before, this indicates that the first function is running the Redis constructor for the subscriber type and functions called after do not run the Redis constructor. This behaviour of having the Redis constructor run only once for each type is the re-use that you are talking about. Regardless of how many queues are created and functions are called on those queues, there are only ever 2 log lines printed and only 2 Redis connections created for client and subscriber.

Another approach to debugging code and determining what has been run is to use a spy. In this case I am using the spy from sinon. Attaching a spy to another object allows detailed inspection of that object and how it has been used. This is what I set up in the second example I provided. Attaching a spy to redis and using that as the contructor for client and subscriber allow access to the Redis constructor to be inspected. Creating and calling functions on queues works as normally and access to the Redis constructor is recorded by the spy. The code SpyRedis.getCalls().map(({args}) => args) prints the different calls that the constructor has received and the arguments used for those calls. Similarly to the console logging, this list indicates how many times a Redis object was constructed. No matter how many queues are created and functions on those queues are called, the number of calls that are printed from the spy is never higher than 2. This indicates one Redis constructor is used for each of the client and subscriber type. Again, this corroborates the claims that this code does cache the Redis connections and re-uses them across different queues as you describe.

If there is further explanation you require I am more than happy to oblige. However, I would encourage you to run this code yourself. That way, you can determine for yourself how this code and other bull code works and if it does the things you expect.

manast · 2021-11-12T15:32:48Z

Thanks for the long explanation, but it did not answer my question, which was, "what is your explanation for this (behaviour)? As in, how is it possible that the redis instances are cached in the "opts" object that you are passing to the queue.

Anyway, it seems that the culprit is the memoization that is working on a shallow copy of redis options:

return function(type) {
    return function() {
      // Memoized connection
      if (connections[type] != null) {
        return connections[type];
      }
      const clientOptions = _.assign({}, options.redis);
      clientOptions.connectionName = this.clientName();
      const client = (connections[type] = createClient(type, clientOptions));

So when instantiating another queue with the same opts object, results in that it returns the same redis instance. This is a bug that should be resolved by deep cloning the options before memoizing...

The documentation text is therefore correct, and it is the proper way to reuse connections, whereas current behaviour is a bug.

bakkerthehacker · 2021-11-12T16:12:07Z

how is it possible that the redis instances are cached in the "opts" object that you are passing to the queue.

The scope that the "opts" object is created in has 2 variables declared as let (client and subscriber). The opts object can reference these variables, read from them and assign a new value to them when they are falsey. This is the mechanism that is used to cache the redis instances across different queues.

The documentation text is therefore correct, and it is the proper way to reuse connections, whereas current behaviour is a bug.

The existing code in the patterns docs does not work and produces an error when run. The caching setup I added here exists only to delay creating the Redis object until the redisOpts object is avaible from the createClient. This opts object contains the maxRetriesPerRequest and enableReadyCheck which are now a requirement for redis clients. Failing to pass these options into Redis, as the existing code in the docs already does, an error is thrown.

So when instantiating another queue with the same opts object, results in that it returns the same redis instance

Neither the existing code in the docs, nor my updates handle memoizing different opts correctly.

Again I encourage you to run the code and see what happens. The code provided in the docs does not work. If this is a bug somewhere else, I would be more than happy to have a fix setup. But they way I see it, the only way this check will pass is if the createClient directly above it actually passes in the redis opts (including maxRetriesPerRequest and enableReadyCheck) to new Redis(). The existing code snippet in the docs does not do this.

manast · 2021-11-12T16:34:06Z

Ok, sorry you are right, I did not read your changes correctly.

manast · 2021-11-16T08:22:32Z

🎉 This PR is included in version 4.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Update pattern for reusing redis connection

e0c55d8

Ensure opts are always passed into redis, allowing for enableReadyCheck and maxRetriesPerRequest to be updated correctly for v4

bakkerthehacker force-pushed the patch-2 branch from bfd9a0c to e0c55d8 Compare November 8, 2021 17:57

manast merged commit b6d530f into OptimalBits:develop Nov 12, 2021

manast added the released label Nov 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update patterns with fix for v4 #2208

Update patterns with fix for v4 #2208

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Update patterns with fix for v4 #2208

Update patterns with fix for v4 #2208

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!