Add connection lifetime limit option and tests

[ChrisWritable]

Here's a rebased version based on your comments here: #2668 (comment)

[brianc]

Thanks! Will merge after tests pass. ❤️

[polomsky]

Because of missing unref, this effectively disables allowExitOnIdle feature.

[ChrisWritable]

Does the unit test need to be updated? The test passed.

✓ unrefs the connections and timeouts so the program can exit when idle when the allowExitOnIdle option is set (151ms)

[polomsky]

Yes. maxLifetimeSeconds should be set there to some higher value, e.g. 60.

[hempels]

Actually I think an additional test is needed in idle-timeout.js since allowExitOnIdle should be tested under both conditions (maxLifetimeSeconds is disabled and maxLifetimeSeconds is a longer time period than idleTimeoutMillis.)

[marcbachmann]

Sorry to also comment on this, but this is preventing a clean shutdown even when client.end() is called.
Can't we just use .unref() on the timeout or

const timeout = setTimeout(....)
client.once('end', () => clearTimeout(timeout))

Maybe even both

Edit: Something like that

          const maxLifetimeTid = setTimeout(() => {
            this.log('ending client due to expired lifetime')
            this._expired.add(client)
            const idleIndex = this._idle.findIndex((idleItem) => idleItem.client === client)
            if (idleIndex !== -1) {
              this._acquireClient(
                client,
                new PendingItem((err, client, clientRelease) => clientRelease()),
                idleListener,
                false
              )
            }
          }, this.options.maxLifetimeSeconds * 1000)

          client.once('end', () => clearTimeout(maxLifetimeTid))

          if (this.options.allowExitOnIdle) {
            maxLifetimeTid.unref()
          }

[hempels]

It's probably worth noting that the default setting for maxLifetimeSeconds is 0 (aka: disabled) and none of these issues appear unless you set it to a non-zero value. That doesn't resolve the issues, but it is important to be aware of since there is a simple workaround (set it back to 0.) It seems like the need to utilize maxLifetimeSeconds would be minimal in cases where clients are being explicitly disconnected (the feature was added primarily to support long-running services in load-balanced DB environments.)

[marcbachmann]

Exactly my use case (for faster recovery after a failover to a replica) 😊
I just ran into some issue with some local scripts, we're I'm using the same config. I might do a PR to fix it in the next few days, unless anybody else is on it. But haven't seen any PRs.