Timeout breaking change

[artur-ma]

The bug was introduced in v6.20, the timeout that is set less than a second is ignored, Its a breaking change in minor version

server

const Fastify = require('fastify')
const fastify = Fastify()

fastify.all('*', async (req, reply) => {
  await new Promise((resolve) => setTimeout(resolve, 500))
  return reply.status(200).send(req.url)
});


fastify.listen({ port: 3000 })

client

const undici = require('undici')

undici.request('http://localhost:3000', {
  headersTimeout: 300,
}).then((response) => {
  console.log(response.statusCode)
})

[mcollina]

I'm relatively certain that this breaking change was introduced in #3656, to fix some really painful problems that were happening in AWS Lambda.

I don't think we can revert those fixes.

If you find a way to fix this so that sub-second timeouts are handled correctly, that's be a blast.

cc @Uzlopak

[artur-ma]

@mcollina since undici is aiming to replace the nodejs http lib, basically an infrastructure for http calls, its odd to fix very specific usecase reported here and ignore all the needs of undici users, more than that, doing it as minor affect the trust users put in undici

There is a variable that contolls the resolution, in fast timers.. The question how will it affect perfromance if it will be decreased

[mcollina]

Sorry the linked pr is incorrect. The actual PR is #3495. As you can see for the linked issues, it fixes many critical issues developers were having with fetch and undici in general in high pressure scenarios.

headersTimeout is a security feature necessary to protect against slowris-like attacks. A sub-second value is absolutely an overkill. Most libraries don't even offer this.

Apart from the unexpected breaking change, what problems is this causing?

[artur-ma]

@mcollina

1. It causes a timeout in higher entity of our request chain ServiceA(timeout caused here) -> ServiceB -> ServiceC
2. It does not allow effective retry strategy on timeout

Isnt it strange to assume no one needs lower timeout value ?

[mcollina]

No one said it's not a problem and people should not have a timeout < 1s for their HTTP requests.

I'm saying that:

1. headersTimeout is a security mechanism. Anything lower than tens of seconds is not is not its intended purpose.
2. there are no guarantee when a timer will fire in Node.js, as it's not a real-time system. Unfortunately, this meant quite a few issues under busy event loop scenarios.
3. handling the passing of time is one of the biggest bottleneck of doing http requests in Node.js, and we have to do a custom implementation, otherwise performance will tank.

Now, that change was unfortunately breaking, as no one spotted it earlier on, and it's poorly documented. I'll add some docs about it.

Having said all of that, I won't have issues with a PR that fixes this regression, but given the nature of the breaking change, and the feature in question, I'm not going to spend time on it.

(It's also super easy to work around the problem in userland with a setTimeout.. and incur in the problems mentioned above in busy scenarios).

[artur-ma]

Having said all of that, I won't have issues with a PR that fixes this regression, but given the nature of the breaking change, and the feature in question, I'm not going to spend time on it.

As I said before, there is RESOLUTION_MS const, that controlls it, I can change it, the question is how will it affect performance

also it was spotted before, #3798

(It's also super easy to work around the problem in userland with a setTimeout.. and incur in the problems mentioned above in busy scenarios).

Not sure about the workaround, as I want to know when my service handled the request, but didn't write the response to socket yet, because im not interested in response body, and I do not want to wait for it to be written to the socket

[Uzlopak]

Interesting. I will have a look at it.