💥 Bump redis version to 5.0.0

[dawidreedsy]

Redis released version 5 , but the only breaking change that is affecting us is:

Connection Management:

client.QUIT/quit() is replaced by client.close()
client.disconnect() has been renamed to client.destroy()

I tried to make it a non breakable change, by allowing both close and quit

[dawidreedsy]

It looks like now redis is waiting for all the pending commands to finish then only destroys the socket:
https://github.com/redis/node-redis/blob/47e297077a20f396a1b22fafa917cea284dc3427/packages/client/lib/client/index.ts#L1198-L1219

Not yet sure what command is pending in tests though. This is the command from the wait queue

{
  args: undefined,
  chainId: undefined,
  abort: undefined,
  resolve: [Function: resolve],
  reject: [Function: reject],
  channelsCounter: 1,
  typeMapping: {
    '36': [Function: Buffer] {
      poolSize: 8192,
      from: [Function: from],
      copyBytesFrom: [Function: copyBytesFrom],
      of: [Function: of],
      alloc: [Function: alloc],
      allocUnsafe: [Function: allocUnsafe],
      allocUnsafeSlow: [Function: allocUnsafeSlow],
      isBuffer: [Function: isBuffer],
      compare: [Function: compare],
      isEncoding: [Function: isEncoding],
      concat: [Function: concat],
      byteLength: [Function: byteLength],
      [Symbol(kIsEncodingSymbol)]: [Function: isEncoding]
    }
  }
}

The weird part here is that args is undefined,

It might be relatred to publish command though

sendCommand [
  'EVAL',
  'for i = 2, #ARGV do redis.call("publish", ARGV[i], ARGV[1]) end',
  '0',
  '{"test":true}',
  'x',
  'y'
]

[alecgibson]

The issue here is that there's a bug where we're eagerly calling close() (which to be fair existed before 🙈 ), since the contents of Promise.all() are immediately invoked because we're missing a closure inside .then(). I think this function should read:

RedisPubSub.prototype._close = function() {
  var pubsub = this;
  return this._closing = this._closing || this._connect().then(function() {
    return Promise.all([
      close(pubsub.client),
      close(pubsub.observer)
    ]);
  });
};

I tried this locally and it seems to solve the socket hangup issue.

[dawidreedsy]

Thats a good spot!

[alecgibson]

I think technically .quit() is only deprecated for Redis server >=7.2:

As of Redis version 7.2.0, this command is regarded as deprecated.

It can be replaced by just closing the connection when migrating or writing new code.

I think I'd read this notice as anyone using Redis server <7.2 should still use .quit(), regardless of Node.js client version(?).

Not sure how we want to handle this?

[dawidreedsy]

This is the note in the Redis docs:

Note: Clients should not use this command. Instead, clients should simply close the connection when they're not used anymore. Terminating a connection on the client side is preferable, as it eliminates TIME_WAIT lingering sockets on the server side.

The redis-node code does seem to just close the socket connection. I guess it is the same thing as closing connection to internet.

[dawidreedsy]

@alecgibson @ericyhwang
This is a breaking change anyway:

• 💥 Drop support for version 16 and 18 of node #25

Maybe we can just drop support for node-redis < 5.0.0 then we can just use close.

[alecgibson]

Okay turns out redis@5 doesn't support Redis server <7.2, so I guess it doesn't matter if older server users should still use .quit(), because those consumers should still be on redis@4.

[alecgibson]

I think we shouldn't make this change breaking. It's nice to let people use the older version if possible, especially because it makes the migration path easier to keep the shim around. And since it's super easy for us, I don't see why not.

[alecgibson]

I'd consider this a minor patch given that it adds functionality

[dawidreedsy]

Change to 5.1.0

[alecgibson]

Let's deal with Node.js versions separately please. eg I think 16 is EOL, and we should consider that change separately (as a major bump)

[dawidreedsy]

Removed

[coveralls]

coverage: 100.0%. remained the same
when pulling abc1d92 on bump-redis-version
into 819b549 on master.