Fix: resolve doubly linked list push issue

[ntvviktor]

Description

Describe your pull request here

• Fixed initialization problem in DoublyLinkedList.push() for an empty doubly linked list
• Resolves issue with client pool management
• An attempt to fix this issue Unhandled promise rejection triggered in DoublyLinkedList after executeTask() #3062.

Now the client pool properly shut down after getting a bad connection.

Checklist

• Does npm test pass with this change (including linting)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)? Just a Bug fix.

[nkaradzhov]

@ntvviktor looks like you broke the tests

[ntvviktor]

@ntvviktor looks like you broke the tests

hey nkaradzhov, I am considering, even though fixing the code logic of the DoublyLinkedList will properly shut down the bad host request for redis connection, but it will break the sentinel client tests. I believe it has been somehow coded to pass with the error of issue:

TypeError: Cannot set properties of undefined (setting 'next')
  File "/app/node_modules/.pnpm/@redis+client@5.8.0/node_modules/@redis/client/dist/lib/client/linked-list.js", line 76, col 32, in DoublyLinkedList.remove
    node.previous.next = node.next;

I will investigate further to try to triage and fix the root cause.

[ntvviktor]

In the case of the issue above, seems like the RedisClientPool use the Doubly Linked List for a client in use store, so when there is an error such as bad host request as the issue described, the bang mark node.previous!.next in the remove() function in DoublyLinkedList lead to the TypeError since it calls remove() twice, in the catch, and in the #returnClient.

async #create() {
    const node = this._self.#clientsInUse.push(
      this._self.#clientFactory()
        .on('error', (err: Error) => this.emit('error', err))
    );

    try {
      const client = node.value;
      await client.connect();
    } catch (err) {
      this._self.#clientsInUse.remove(node);
      throw err;
    }

    this._self.#returnClient(node);
  }

I fix using the if statement to check for the previous is not enough, and found out a root cause from RedisCommandQueue also using Doubly Linked List.
It uses a for loop statement over an iterator *nodes() function while using remove() in the loop, leading to a TimeOut command execution in the Test, so in this PR I also trying to fix that by changing the iterator function *nodes() to make sure setting node.next to undefined at shift(), unshift(), and remove() will be safe.

I ran all tests locally and it currently works as expected. @nkaradzhov Please help me enable the pipeline to see whether my fix break anything. Thanks!

[ntvviktor]

Hi @nkaradzhov, please help me review the code, thanks in advance

[nkaradzhov]

@ntvviktor the PR looks to be damaged somehow, it looks like ther are some unwanted changes after a merge?

[ntvviktor]

@ntvviktor the PR looks to be damaged somehow, it looks like ther are some unwanted changes after a merge?

Hi @nkaradzhov thanks for feedback, I have made changes, my bad on the rebase master branch into this.

[nkaradzhov]

@ntvviktor sorry for being paranoid here, but could you please provide a test case for the DoublyLinkedList somewhere here
Usually, when we find a bug, we write a test that proves the bug, then do a fix and the test starts passing. We end up having our test suite growing stronger

[ntvviktor]

@ntvviktor sorry for being paranoid here, but could you please provide a test case for the DoublyLinkedList somewhere here Usually, when we find a bug, we write a test that proves the bug, then do a fix and the test starts passing. We end up having our test suite growing stronger

Thanks @nkaradzhov, I have added the according tests.

[nkaradzhov]

CLGTM

[nkaradzhov]

Looks like this causes #3127

[nkaradzhov]

@ntvviktor, sorry, what was the reason for the addition of #handleSentinelFailure?

[ntvviktor]

Hey @nkaradzhov, it was because during this test case, after a master is shutdown and the test trying to reconnect, the sentinelSentinels or some other utils could use the first item [0] in the list, but it already shutdown, hence the topology-change was never reached or emitted, only the master-change was? hence, I made changes by remove the dead node before the #reset function so that it could has the correct event emitted, I personally think the #reset function would bring back the re-connecting for all of the redis sentinels, but perhaps I made a wrong assumption. Let me know what you think

it('shutdown sentinel node', async function () {
      this.timeout(60000);
      sentinel = frame.getSentinelClient();
      sentinel.setTracer(tracer);
      sentinel.on("error", () => { });
      await sentinel.connect();
      tracer.push("connected");

      let sentinelChangeResolve;
      const sentinelChangePromise = new Promise((res) => {
        sentinelChangeResolve = res;
      })

      const sentinelNode = sentinel.getSentinelNode();
      tracer.push(`sentinelNode = ${sentinelNode?.port}`)

      sentinel.on('topology-change', (event: RedisSentinelEvent) => {
        tracer.push(`got topology-change event: ${JSON.stringify(event)}`);
        if (event.type === "SENTINEL_CHANGE") {
          tracer.push("got sentinel change event");
          sentinelChangeResolve(event.node);
        }
      });

      tracer.push("Stopping sentinel node");
      await frame.stopSentinel(sentinelNode!.port.toString());
      tracer.push("Stopped sentinel node and waiting on sentinel change promise");
      const newSentinel = await sentinelChangePromise as RedisNode;
      tracer.push("got sentinel change promise");
      assert.notEqual(sentinelNode!.port, newSentinel.port);
    });

when the frame trying to get