fix: enhance 100 and 200 or 204 handling

[Iamshankhadeep]

…zationHook`

fixes #5003

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[Uzlopak]

So potential performance hit, because some people apply a preSerializiation hook on 204?

[mcollina]

Thanks for opening a PR! Can you please add a unit test?

[jsumners]

Why are there unnecessary white space changes?

[Iamshankhadeep]

@mcollina @jsumners @Uzlopak can you review this ?

[mcollina]

lgtm

[Iamshankhadeep]

why did the integration test failed?

[metcoder95]

It seems the last test is failing, can you try to replicate it in local?

fastify/integration/server.js

Line 24 in 5a98e7e

fastify.delete('/delete/:id', async function (request, reply) {

[github-actions]

Node: 16
PR: [1] 1692k requests in 30.09s, 318 MB read
MAIN: [1] 1639k requests in 30.05s, 308 MB read

---

Node: 18
PR: [1] 1656k requests in 30.05s, 311 MB read
MAIN: [1] 1653k requests in 30.06s, 311 MB read

---

Node: 20
PR: [1] 1453k requests in 30.07s, 273 MB read
MAIN: [1] 1523k requests in 30.08s, 286 MB read

[Iamshankhadeep]

It seems the last test is failing, can you try to replicate it in local?

fastify/integration/server.js

Line 24 in 5a98e7e

fastify.delete('/delete/:id', async function (request, reply) {

so getting a error for this, when trying to send data in 204 status.

{
    "statusCode": 500,
    "code": "FST_ERR_REP_INVALID_PAYLOAD_TYPE",
    "error": "Internal Server Error",
    "message": "Attempted to send payload of invalid type 'object'. Expected a string or Buffer."
}

the correct code should be this right?

fastify.delete('/delete/:id', async function (request, reply) {
  const { id } = request.params
  reply.code(204).send()
})

[metcoder95]

so getting a error for this, when trying to send data in 204 status.

{
    "statusCode": 500,
    "code": "FST_ERR_REP_INVALID_PAYLOAD_TYPE",
    "error": "Internal Server Error",
    "message": "Attempted to send payload of invalid type 'object'. Expected a string or Buffer."
}

the correct code should be this right?

fastify.delete('/delete/:id', async function (request, reply) {
  const { id } = request.params
  reply.code(204).send()
})

Good catch!

Yes, exactly. It seems we didn't notice it before, but with your change, as on 204 we now bypass the serialization step, it goes straight away into preparing the payload to be dispatched and noticed is not a string nor a Buffer, throwing that error.

Can you also add a test

Maybe you'd like to inline it with the payload === undefined check as both scopes are the same. See

[Iamshankhadeep]

fastify/test/close-pipelining.test.js

Line 97 in 14a9ccc

t.equal(responses[1].status, 'fulfilled')

Can anyone help me with this why this gives error, says status: 'rejected', for responses[1].status ?

[climba03003]

Sorry, a bit late for the comment.
But override the payload inside reply.send shouldn't be the right option.
Instead, the guard should be at the last stage before sending to res.write.

The reason is that,

1. User lost the original payload inside onSend hook
2. .send is not the final state for user to change the status code.

[metcoder95]

But override the payload inside reply.send shouldn't be the right option.
Instead, the guard should be at the last stage before sending to res.write.

Good catch, let's do that 👍
This is the line @climba03003 is pointing out:

fastify/lib/reply.js

Line 616 in 14a9ccc

res.write(payload)

[mcollina]

lgtm

[szegheo]

I'm still trying to find the exact reason, but after this change, our logs are full width "response errored" entries. We are heavily using the proxy plugin and had no issues with it until this change. We have only one endpoint, that responds with 204 (PUT), and this is the only one started to (falsely) trigger the above warns. After reading all the comments belonging to this fix, I've double-checked our code (response and headers) but all seems fine on our part. I'm still doing my investigations, just wanted to share my perception.

[szegheo]

I have a reproduction code for my "response errored" issue above (reported in reply-from). The problem is with 204 responses (method independent) and when using node's http request (proxy options).

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.