Skip to content

Conversation

ntvviktor
Copy link

Description

Describe your pull request here

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 nkaradzhov self-requested a review September 23, 2025 07:41
@nkaradzhov
Copy link
Collaborator

@ntvviktor looks like you broke the tests

@ntvviktor
Copy link
Author

ntvviktor commented Sep 24, 2025

@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/@[email protected]/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
Copy link
Author

ntvviktor commented Sep 25, 2025

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants