Undici throws a ConnectTimeoutError when there is no connection

[mdmitry01]

Bug Description

If you try to send an HTTP request, but the DNS lookup for the requested domain name fails, Undici may throw a ConnectTimeoutError.

Reproducible By

I wrote a simple script which reproduces the issue

const { Pool } = require('undici')

const agent = new Pool('http://foobar.bar')

for (let i = 0; i < 500; i++) {
  agent.request({ method: 'GET', path: '/foobar' })
    .then(async ({ body }) => {
      for await (const data of body) {
        console.log('data', data.toString('utf8'))
      }
    })
    .catch(console.error)
}

I used undici@5.4.0

Expected Behavior

I'd expect to get only errors which indicate that the DNS lookup has failed.

Logs & Screenshots

Kazam_screencast_00184.mp4

Environment

Ubuntu 20.04.4 LTS, Node v16.13.0

[mcollina]

I cannot reproduce the error at all on Linux or Mac.

@ronag could you take a look too?

[jodevsa]

@mcollina I can reproduce it with

const { Pool } = require('undici')

const agent = new Pool('http://foobar.bar')

for (let i = 0; i < 50000; i++) {
  agent.request({ method: 'GET', path: '/foobar' })
    .then(async ({ body }) => {
      for await (const data of body) {
        console.log('data', data.toString('utf8'))
      }
    })
    .catch(console.error)
}

then try to write the output on a file and search for TIMEOUT. something like

node a.js 2> a.txt

[jodevsa]

Seems like the long running loop execution is blocking the event loop from moving the error event callback from the callback queue to the stack which leads to a timeout and the TO callback is appearing in the callback queue before the error event callback because we defer the error event to the next tick: https://github.com/nodejs/node/blob/main/lib/net.js#L1144

a quick test of deferring the definition of the setTimout to the next tick fixes the problem:

let timeoutId = null
    socket
      .setNoDelay(true)
      .once(protocol === 'https:' ? 'secureConnect' : 'connect', function () {
        clearTimeout(timeoutId)

        if (callback) {
          const cb = callback
          callback = null
          cb(null, this)
        }
      })
      .on('error', function (err) {
        clearTimeout(timeoutId)

        if (callback) {
          const cb = callback
          callback = null
          cb(err)
        }
      })

      process.nextTick(()=>{
      timeoutId = timeout
      ? setTimeout(onConnectTimeout, timeout, socket)
      : null
      })

[mcollina]

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

[jodevsa]

sure, I'll prepare a PR ASAP

[jodevsa]

created the following PR: #1551

[jodevsa]

reproduce script:

const { Pool } = require('undici')
const assert = require('assert')
const agent = new Pool('http://foobar.bar')

for (let i = 0; i < 50000; i++) {
  agent.request({ method: 'GET', path: '/foobar' })
    .then(async ({ body }) => {
      for await (const data of body) {
        console.log('data', data.toString('utf8'))
      }
    })
    .catch((e)=>assert.equal(e.code,'ENOTFOUND'))
}