Error handler for aborted request not called reliably

[meyfa]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.14.1

Plugin version

No response

Node.js version

18.15.0

Operating system

Windows

Operating system version (i.e. 20.04, 11.3, 10)

11

Description

In #4611, logic was added to call the error handler when an aborted request is responded to. This broke one of our tests, since in v4.14.0, aborted requests only caused onRequestAbort to be called, but not also the error handler, which is behavior that we depended on.

After some investigation, I found out that the error handler isn't even called reliably:

• When onRequestAbort hook is registered, the error handler is called
• When onRequestAbort hook isn't registered, the error handler is never called

Additionally, the error passed to the error handler is not instanceof Error and doesn't have a statusCode property, unlike indicated by TypeScript types for FastifyError, which the parameter is. In fact, the error given is the response object returned from the route handler. This poses a problem for us because we're logging errors, but the response objects can contain sensitive data, which we don't want to log.

Steps to Reproduce

Code sample for reproduction:

import { fastify } from 'fastify'
import net from 'node:net'
import { setTimeout } from 'node:timers/promises'

const app = fastify()
app.setErrorHandler(async (error, request, reply) => {
  console.log(error, error instanceof Error, error.statusCode)
})
app.get('/', async (req) => {
  console.log('receive start', req.id, req.raw.destroyed)
  await setTimeout(500)
  console.log('receive end', req.id, req.raw.destroyed)
  return { hello: 'world' }
})

// Comment out the following line, the error handler will not be invoked
app.addHook('onRequestAbort', async (req) => console.log('abort', req.id))

await app.listen({ port: 0 })

const socket = net.connect(app.server.address().port)
socket.write('GET / HTTP/1.1\r\nHost: example.com\r\n\r\n')
await setTimeout(200)
socket.destroy()

await setTimeout(1000)
await app.close()

When invoked as stated above, the output is:

> node index.js
receive start req-1 false
abort req-1
receive end req-1 true
{ hello: 'world' } false undefined

With the line commented out:

> node index.js
receive start req-1 false
receive end req-1 true

Expected Behavior

Preferrably, I wouldn't want aborted requests be considered errors, even when a response is sent (i.e. revert #4611). But I do understand the intent of that PR, so if it's not an option, I'd expect the following:

• Call the error handler no matter if onRequestAbort hook is registered
• Pass an instance of FastifyError to the error handler, with a generic error message, instead of the response body

[mcollina]

cc @Eomm @ShogunPanda @kibertoad

[kibertoad]

Personally I don't think that aborted requests should be considered errors, and we shouldn't be invoking errorHandler there, I would suggest to consider this to be a bug.

[ShogunPanda]

Since the request was aborted I don't see a reason to invoke the error handler as, and it is stated in documentation, the socket no longer exists so no data can be sent to the client.

So, to summarize, the overall problem here is that we're invoking the errorHandler and we should not, isn't it?

[meyfa]

So, to summarize, the overall problem here is that we're invoking the errorHandler and we should not, isn't it?

Yes, that's how I see it as well, and it appears everyone in this thread agrees so far.

[mcollina]

I think we are all in agreement, so this is just a bug.

[mcollina]

@kibertoad #4611 was added to solve a bug you reported on twitter:

"a client closes the connection before the entire request has been received"
Maybe I'm misreading the documentation, but this sounds to me that execution hasn't even started yet, so ideally fastify shouldn't even be calling handler to begin with.

https://twitter.com/kibertoad/status/1631687720769658882

So maybe that was not the right fix after all?

Wdyt @Eomm ?

[meyfa]

I opened PR #4631 to resolve this issue. @Eomm's PR #4611 made 3 choices, 2 of which are kept:

• the handler is executed
• the onSend hook is called

The third one is changed according to the above consensus:

• the onError hook is not called when the handler resolves