fix: enhance 100 and 200 or 204 handling by Iamshankhadeep · Pull Request #5056 · fastify/fastify

Iamshankhadeep · 2023-09-23T08:37:08Z

…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

…zationHook`

Uzlopak · 2023-09-23T08:40:34Z

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 · 2023-09-25T12:57:10Z

@mcollina @jsumners @Uzlopak can you review this ?

mcollina

lgtm

Iamshankhadeep · 2023-09-26T07:12:37Z

why did the integration test failed?

metcoder95 · 2023-09-26T07:55:14Z

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 · 2023-09-26T11:18:50Z

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

lib/reply.js

Iamshankhadeep · 2023-09-26T12:59:00Z

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

test/reply-code.test.js

Iamshankhadeep · 2023-09-27T09:54:59Z

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,

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

metcoder95 · 2023-09-28T07:52:59Z

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)

lib/reply.js

test/reply-code.test.js

mcollina

lgtm

lib/reply.js

szegheo · 2023-12-17T10:56:38Z

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 · 2023-12-21T08:10:13Z

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 · 2024-12-21T00:27:24Z

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.

fix: status code is between 100 and 200 or 204 don't call `preSeriali…

b35bacc

…zationHook`

mcollina requested changes Sep 23, 2023

View reviewed changes

jsumners reviewed Sep 23, 2023

View reviewed changes

metcoder95 and others added 4 commits September 25, 2023 09:48

Merge branch 'main' into status-204

7c3504f

feat: add test for status code 204 and fix spaces

151a9f7

fix: spaces

5a7c991

fixes: spaces

09513f1

Iamshankhadeep requested review from jsumners and mcollina September 25, 2023 12:56

mcollina approved these changes Sep 25, 2023

View reviewed changes

gurgunday approved these changes Sep 25, 2023

View reviewed changes

metcoder95 approved these changes Sep 26, 2023

View reviewed changes

Fdawgs added the benchmark Label to run benchmark against PR and main branch label Sep 26, 2023

jsumners reviewed Sep 26, 2023

View reviewed changes

lib/reply.js Outdated Show resolved Hide resolved

Fdawgs removed the benchmark Label to run benchmark against PR and main branch label Sep 26, 2023

metcoder95 reviewed Sep 26, 2023

View reviewed changes

test/reply-code.test.js Show resolved Hide resolved

Iamshankhadeep and others added 2 commits September 27, 2023 10:40

feat: no payload is send in status 204 and 1xx

ae1b2cf

Merge branch 'main' into status-204

4f65b7b

climba03003 requested changes Sep 28, 2023

View reviewed changes

feat: check for status code in onSendEnd

4000599

Iamshankhadeep requested review from climba03003 and metcoder95 September 28, 2023 08:33

Iamshankhadeep requested review from gurgunday, jsumners and mcollina September 28, 2023 08:33

metcoder95 reviewed Sep 28, 2023

View reviewed changes

lib/reply.js Show resolved Hide resolved

test/reply-code.test.js Show resolved Hide resolved

test/reply-code.test.js Show resolved Hide resolved

feat: onSend hook can't alter the payload

d9117f0

Iamshankhadeep requested a review from metcoder95 September 28, 2023 20:24

mcollina approved these changes Oct 3, 2023

View reviewed changes

Merge branch 'main' into status-204

6ec42b0

gurgunday approved these changes Oct 3, 2023

View reviewed changes

metcoder95 approved these changes Oct 3, 2023

View reviewed changes

lib/reply.js Show resolved Hide resolved

climba03003 approved these changes Oct 4, 2023

View reviewed changes

metcoder95 changed the title ~~fix: status code is between 100 and 200 or 204 don't call `preSeriali…~~ fix: enhance 100 and 200 or 204 handling Oct 4, 2023

metcoder95 merged commit cefe85d into fastify:main Oct 4, 2023

renovate bot mentioned this pull request Oct 13, 2023

fix(deps): update dependencies aicore/cocodb#100

Merged

1 task

Eomm mentioned this pull request Dec 18, 2023

CI seems to be failing fastify/fastify-request-context#175

Closed

szegheo mentioned this pull request Dec 20, 2023

False response errored messages fastify/fastify-reply-from#338

Closed

2 tasks

github-actions bot locked as resolved and limited conversation to collaborators Dec 21, 2024

Uh oh!

Conversation

Iamshankhadeep commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uzlopak commented Sep 23, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jsumners left a comment

Choose a reason for hiding this comment

Uh oh!

Iamshankhadeep commented Sep 25, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Iamshankhadeep commented Sep 26, 2023

Uh oh!

metcoder95 commented Sep 26, 2023

Uh oh!

github-actions bot commented Sep 26, 2023

Uh oh!

Uh oh!

Iamshankhadeep commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Iamshankhadeep commented Sep 27, 2023

Uh oh!

climba03003 left a comment

Choose a reason for hiding this comment

Uh oh!

metcoder95 commented Sep 28, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szegheo commented Dec 17, 2023

Uh oh!

szegheo commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Iamshankhadeep commented Sep 23, 2023 •

edited

Loading

Iamshankhadeep commented Sep 26, 2023 •

edited

Loading

szegheo commented Dec 21, 2023 •

edited

Loading