Handle response validation error by marcobiraghi · Pull Request #1328 · fastify/fastify

marcobiraghi · 2018-12-14T15:11:33Z

Fix for issue #1325 : Call onErrorHook if response serialization throw an error

My branch

┌─────────┬──────┬──────┬───────┬───────┬─────────┬──────────┬───────────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%   │ Avg     │ Stdev    │ Max       │
├─────────┼──────┼──────┼───────┼───────┼─────────┼──────────┼───────────┤
│ Latency │ 0 ms │ 0 ms │ 75 ms │ 81 ms │ 6.82 ms │ 21.98 ms │ 352.12 ms │
└─────────┴──────┴──────┴───────┴───────┴─────────┴──────────┴───────────┘
┌───────────┬─────────┬─────────┬────────┬─────────┬─────────┬─────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%    │ 97.5%   │ Avg     │ Stdev   │ Min     │
├───────────┼─────────┼─────────┼────────┼─────────┼─────────┼─────────┼─────────┤
│ Req/Sec   │ 10111   │ 10111   │ 13383  │ 17567   │ 14242.4 │ 2729.37 │ 10111   │
├───────────┼─────────┼─────────┼────────┼─────────┼─────────┼─────────┼─────────┤
│ Bytes/Sec │ 1.66 MB │ 1.66 MB │ 2.2 MB │ 2.88 MB │ 2.34 MB │ 448 kB  │ 1.66 MB │
└───────────┴─────────┴─────────┴────────┴─────────┴─────────┴─────────┴─────────┘

master

┌─────────┬──────┬──────┬───────┬───────┬────────┬──────────┬──────────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%   │ Avg    │ Stdev    │ Max      │
├─────────┼──────┼──────┼───────┼───────┼────────┼──────────┼──────────┤
│ Latency │ 0 ms │ 0 ms │ 76 ms │ 82 ms │ 6.8 ms │ 21.86 ms │ 344.8 ms │
└─────────┴──────┴──────┴───────┴───────┴────────┴──────────┴──────────┘
┌───────────┬────────┬────────┬────────┬─────────┬─────────┬─────────┬─────────┐
│ Stat      │ 1%     │ 2.5%   │ 50%    │ 97.5%   │ Avg     │ Stdev   │ Min     │
├───────────┼────────┼────────┼────────┼─────────┼─────────┼─────────┼─────────┤
│ Req/Sec   │ 9727   │ 9727   │ 13407  │ 17839   │ 14287.2 │ 3029.42 │ 9725    │
├───────────┼────────┼────────┼────────┼─────────┼─────────┼─────────┼─────────┤
│ Bytes/Sec │ 1.6 MB │ 1.6 MB │ 2.2 MB │ 2.92 MB │ 2.34 MB │ 496 kB  │ 1.59 MB │
└───────────┴────────┴────────┴────────┴─────────┴─────────┴─────────┴─────────┘

After fix, performance are 97,2% of actual.

I'm not sure of try-catch solution, but I think it's the less impacting way on the code base. I'm open to better solutions.

Call onErrorHook if response serialization throw an error

mcollina · 2018-12-14T15:27:26Z

lib/reply.js

+    } catch (error) {
+      onErrorHook(this, error, onSendHook)
+      return
+    }


I think a better approach would be to reset the sent to false and the let the exception bubble up to the user.

Do you suggest to reset sent in the catch block (instead of call onErrorHook) or set sent = true just a little before to call onSendHook?
The first approach keeps the performance degradation, so the second one appears better to me, but I haven't a deep knowledge of Fastify codebase and I cannot evaluate if this could have some side effects...

This change is not really related to performance, but correctness. We should not be invoking here onErrorHook, but rather bubble the error to the user. We'll focus on optimizing after that change.

Note that the call to this[kReplySerializer](payload) would need to be protected in a similar way.

I changed the way sent is set, to cover all cases without try-catches. Benchmark updated.

mcollina · 2018-12-14T15:27:40Z

test/validation-error-handling.test.js

+  const fastify = Fastify()
+
+  fastify.get('/', { schema: { response } }, function (req, reply) {
+    reply.code(200).send({ work: 'actor' })


I think the exception should bubble up here. This is a developer error.

I'm not sure I like this, in my opinion send should never throw. Otherwise, we are "suggesting" users to always put send inside a try-catch block.

@delvedor this is a philosophical decision 😄 We can hide the error and handle it with onErrorHook (see my first commit) or we can makes the error bubble up (my second commit).
1st approach
We wouldn't handle a developer error, like @mcollina say, so Fastify shouldn't hide it. This isn't a common situation and surely caused by a bug. It have to be fixed by developer and not hidden try-catching everything.
2nd approach
By the way, a big project has surely a good amount of bugs, and it's not nice that app crashes due to a missing field. A 500 error is really welcome in production. That's my case, I find this bug due to a malformed record in my MongoDB that caused the validation failure.

What if we let developer decide? With a validator option or something similar. I'm just saying, I'm not sure this match the project philosophy.

I had a chant with @mcollina, and I agree with him.
We must be consistent with our current behavior if you are using a standard function as handler reply.send should throw and you should handle the exception, while if you are using an async handler, we should handle the exception internally.

an async handler will catch the exception, we have to do nothing about it, just throw.

mcollina · 2018-12-14T16:25:04Z

I think we can move reply.sent = true to onSendHook and onErrorHook.

marcobiraghi · 2018-12-14T22:06:27Z

I think we can move reply.sent = true to onSendHook and onErrorHook.

@mcollina I'm moving sent = true in the hooks... but look at here:
https://github.com/fastify/fastify/blob/master/lib/reply.js#L249
https://github.com/fastify/fastify/blob/master/lib/reply.js#L365

It looks like it's not necessary to set sent = true in reply.send() method, because this is already done later in the flow. I double checked all possible paths but those hooks are always called. So I think the fix can be simply a removal of this line https://github.com/fastify/fastify/blob/master/lib/reply.js#L73

mcollina · 2018-12-14T22:10:13Z

Is it passing the tests?

marcobiraghi · 2018-12-15T16:11:33Z

Is it passing the tests?

The best solution is to move the sent = true inside the hooks, as you said. I will investigate if there is a possibility to improve the sent logic, because sometimes it switch from true-false-true many times. But I think this isn't the topic of this PR, so I let the bug fixed only for now.

mcollina

LGTM

mcollina · 2018-12-18T17:59:34Z

I see no difference in performance between this and master.

delvedor

Can you also add a test with promises?

marcobiraghi · 2018-12-19T14:16:28Z

@delvedor Do you mean something like this?

fastify.get('/', { schema: { response } }, async function (req, reply) {
  return Promise.resolve()
    .then(() => reply.code(200).send({ work: 'actor' }))
    .catch((error) =>  reply.code(500).send(error))
})

delvedor · 2018-12-19T15:57:49Z

Something like the following should work.

// note that I'm not using an async function
fastify.get('/', { schema: { response } }, function (req, reply) {
  return Promise.resolve({ work: 'actor' })
}

marcobiraghi · 2018-12-19T16:58:22Z

@delvedor done! 😄

delvedor

LGTM

* Handle response validation error Call onErrorHook if response serialization throw an error * Remove try catch and set sent just before call hooks * Move sent true inside hooks * Add test with promise for response validation error

github-actions · 2022-02-11T00:14:27Z

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.

Handle response validation error

b216aa5

Call onErrorHook if response serialization throw an error

mcollina reviewed Dec 14, 2018

View reviewed changes

Remove try catch and set sent just before call hooks

96d4252

cemremengu added bugfix Issue or PR that should land as semver patch backport 1.x Issue or pr that should be backported to Fastify v1 labels Dec 14, 2018

mcollina mentioned this pull request Dec 15, 2018

Do not allow the reply.sent property to be overwritten #1330

Closed

Move sent true inside hooks

aec576d

mcollina approved these changes Dec 18, 2018

View reviewed changes

delvedor reviewed Dec 18, 2018

View reviewed changes

mcollina mentioned this pull request Dec 19, 2018

Establish reply.sent = true as a proper way to skip async/await handling #1336

Merged

4 tasks

Add test with promise for response validation error

054550d

delvedor approved these changes Dec 19, 2018

View reviewed changes

mcollina merged commit 249bd4a into fastify:master Dec 19, 2018

mcollina mentioned this pull request Dec 20, 2018

Backport sent fixes #1339

Merged

4 tasks

marcobiraghi deleted the handle-response-validation-error branch January 21, 2019 17:01

github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2022

Uh oh!

Conversation

marcobiraghi commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Dec 14, 2018

Uh oh!

marcobiraghi commented Dec 14, 2018

Uh oh!

mcollina commented Dec 14, 2018

Uh oh!

marcobiraghi commented Dec 15, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Dec 18, 2018

Uh oh!

delvedor left a comment

Choose a reason for hiding this comment

Uh oh!

marcobiraghi commented Dec 19, 2018

Uh oh!

delvedor commented Dec 19, 2018

Uh oh!

marcobiraghi commented Dec 19, 2018

Uh oh!

delvedor left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marcobiraghi commented Dec 14, 2018 •

edited

Loading