socketTimeout overwrites connectTimeout

[ekarademir]

Description

When both options are set for sockets, socketTimeout overwrites the connect timeout..

Timeout for socket is set with connectTimeout here, then it's set again with socketTimeout here.

When preciously there was a listener for the connect event, after which the timeout is set, there is nothing now. And I believe that's the reason behind #2855

Node.js Version

v20.9.0

Redis Server Version

6

Node Redis Version

v5.6.0

Platform

MacOS

Logs

[nkaradzhov]

@ekarademir although admittedly the code looks very strange, its working correctly. Let me copy-paste the code here and annotate:

    let onTimeout;
    if (this.#connectTimeout !== undefined) {
      onTimeout = () => socket.destroy(new ConnectionTimeoutError());
      socket.once('timeout', onTimeout);
      socket.setTimeout(this.#connectTimeout);       // <--- initially set timeout as the connectTimeout
    }

    if (this.#isSocketUnrefed) {
      socket.unref();
    }

    await once(socket, this.#socketFactory.event);   // <--- wait for the `connect` event

    if (onTimeout) {
      socket.removeListener('timeout', onTimeout);   // <---- after successful connect, remove the connectTimeout
    }

    if (this.#socketTimeout) {
      socket.once('timeout', () => {
        socket.destroy(new SocketTimeoutError(this.#socketTimeout!));
      });
      socket.setTimeout(this.#socketTimeout);   // <--- set socketTimeout
    }

Just to summarize what the code does:

create socket -> set connectTimeout -> connect -> remove connectTimeout -> set socketTimeout

If the socket fails to connect before the connectTimeout is triggered, then a ConnectionTimeoutError error is thrown

The issue you are referring to is very old and is probably not reproducing anymore. I will be happy to take a look if you have any actual steps to reproduce an issue

[nkaradzhov]

@ekarademir looking again through your comment, i think i understand where the confusion is coming from:

await once(socket, this.#socketFactory.event); <-------- this is where we are waiting for the connect event.

The factory can create multiple types of sockets and not all of them emit the same connect event:

TCP socket emits connect
IPC socket emits connect
TLS socket emits secureConnect

So the factory dynamically returns object that has the event name inside. Hope this clarifies it

[ekarademir]

Thank you for the explanation @nkaradzhov !