Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Encapsulated error handling #3261

Merged
merged 11 commits into from Aug 20, 2021
Merged

Encapsulated error handling #3261

merged 11 commits into from Aug 20, 2021

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Aug 17, 2021 •
edited

This PR improves the error handling by allowing encapsulated error handler to call the upper-level error handler (or the fallback handler).

Ref #2860
Fixes #1436

Checklist

@mcollina mcollina added the semver-major Issue or PR that should land as semver major label Aug 17, 2021
@mcollina mcollina added this to the v4.0.0 milestone Aug 17, 2021
@mcollina mcollina requested review from jsumners, delvedor, Eomm and a team August 17, 2021 07:28
@mcollina
Copy link
Member Author

mcollina commented Aug 17, 2021 •
edited

@fastify/core I opened this one as draft to get early feedback. I would work to update the docs asap. All my console.logs are still there for ease of future debugging in this PR - I'll get this cleaned up.

Let me know what you think.

@sergiz PTAL

kibertoad
kibertoad reviewed Aug 17, 2021
View reviewed changes
lib/error-handler.js Outdated Show resolved Hide resolved
kibertoad
kibertoad reviewed Aug 17, 2021
View reviewed changes
lib/error-handler.js Outdated Show resolved Hide resolved
kibertoad
kibertoad reviewed Aug 17, 2021
View reviewed changes
lib/error-handler.js Outdated Show resolved Hide resolved
kibertoad
kibertoad reviewed Aug 17, 2021
View reviewed changes
test/reply-error.test.js Outdated Show resolved Hide resolved
Eomm
Eomm reviewed Aug 17, 2021
View reviewed changes
test/hooks.test.js Outdated Show resolved Hide resolved
test/stream.test.js Outdated Show resolved Hide resolved
test/internals/handleRequest.test.js Show resolved Hide resolved
lib/route.js Show resolved Hide resolved
test/logger.test.js Show resolved Hide resolved
jsumners
jsumners reviewed Aug 17, 2021
View reviewed changes
lib/reply.js
@@ -508,7 +496,7 @@ function sendStream (payload, res, reply) {
}

function onErrorHook (reply, error, cb) {
if (reply.context.onError !== null && reply[kReplyErrorHandlerCalled] === true) {
if (reply.context.onError !== null && !reply[kReplyNextErrorHandler]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the coercion check necessary? Or can it be === false?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coercion is necessary. There might be a bug hiding somewhere around this check as it was one of the most troubling spot while doing this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment to that effect.

@mcollina mcollina marked this pull request as ready for review August 18, 2021 07:31
@mcollina
Copy link
Member Author

This is now ready for review - I have just added docs.

@mcollina mcollina changed the title Improve error handling Encapsulated error handling Aug 18, 2021
zekth
zekth approved these changes Aug 18, 2021
View reviewed changes
Copy link
Member

@zekth zekth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment on doc but LGTM.

good job on this!

docs/Errors.md Outdated Show resolved Hide resolved
jsumners
jsumners reviewed Aug 18, 2021
View reviewed changes
docs/Errors.md Outdated Show resolved Hide resolved
mcollina and others added 2 commits August 18, 2021 15:05
@mcollina @jsumners
Update docs/Errors.md
21c33b0 
Co-authored-by: James Sumners <james@sumners.email>
@mcollina
typo
5d1f8e2
fox1t
fox1t approved these changes Aug 18, 2021
View reviewed changes
Copy link
Member

@fox1t fox1t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing feature!

@sergejostir
Copy link
Contributor

sergejostir commented Aug 20, 2021 •
edited

The first message incorrectly states that this PR fixes #2860. It doesn't make any changes in that regard.

fastify/test/encapsulated-error-handler.test.js

Line 13 in 5d1f8e2

throw new Error('catched')

fastify/test/encapsulated-error-handler.test.js

Line 19 in 5d1f8e2

t.equal(err.message, 'catched')

Still incorrect verbs used.

@mcollina
Copy link
Member Author

The first message incorrectly states that this PR fixes #2860. It doesn't make any changes in that regard.

My reading of what is left of that issue is that throwing within error handlers was something not supported before.
This makes it the default mode of operation for calling the parent error handler.

Maybe I misunderstood that issue.

@sergejostir
Copy link
Contributor

The inconsistencies described there are in regards to throwing errors in connection with whether handler/error handler are sync or async. This PR doesn't make any changes in that regard. I will open a PR which addresses these things during the weekend.

@mcollina
Copy link
Member Author

Ok!

Landing this now so you could use as a basis and not conflict.

@mcollina mcollina merged commit 695b1f1 into next Aug 20, 2021
@mcollina mcollina deleted the improve-error-handling branch August 20, 2021 11:04
delvedor
delvedor approved these changes Aug 20, 2021
View reviewed changes
Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A delayed LGTM!

sergejostir added a commit to sergejostir/fastify that referenced this pull request Nov 14, 2021
@sergejostir
fixup! Encapsulated error handling (fastify#3261)
baa1553
sergejostir added a commit to sergejostir/fastify that referenced this pull request Nov 14, 2021
@sergejostir
fixup! Encapsulated error handling (fastify#3261)
2dbc4fb
@sergejostir sergejostir mentioned this pull request Nov 14, 2021
Merged
4 tasks
mcollina pushed a commit that referenced this pull request Nov 23, 2021
@sergejostir
Adjust existing PRs for the new reply.sent handing (#3444)
fd8503a 
* fixup! Encapsulated error handling (#3261)

* fixup! fixed unresponsive server  #3283 (#3285)

* fixup! fix: reply object not marked as sent for stream payloads (#3318)
@mcollina mcollina mentioned this pull request Nov 24, 2021
Closed
@mcollina mcollina mentioned this pull request Jan 25, 2022
Open
@Eomm Eomm mentioned this pull request Jan 28, 2022
Closed
@mcollina mcollina mentioned this pull request May 24, 2022
Merged
4 tasks
mcollina added a commit that referenced this pull request Jun 21, 2022
@mcollina
feat: make sure all validation errors have a statusCode set
0c2baff 
As part of the error handling refactoring of #3261, we should
not be setting a custom status code for validation routes.
We should rely only on the error.statusCode property instead and
leave the user full customization capabilities. Unfortunately
a change was missed.

Ref: #3261
Signed-off-by: Matteo Collina <hello@matteocollina.com>
mcollina added a commit that referenced this pull request Jun 21, 2022
@mcollina
feat: make sure all validation errors have a statusCode set
d52dc43 
As part of the error handling refactoring of #3261, we should
not be setting a custom status code for validation routes.
We should rely only on the error.statusCode property instead and
leave the user full customization capabilities. Unfortunately
a change was missed.

Fixes: #4028
Ref: #3261
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina mcollina mentioned this pull request Jun 21, 2022
Merged
4 tasks
mcollina added a commit that referenced this pull request Jun 21, 2022
@mcollina
feat: make sure all validation errors have a statusCode set
125e5b5 
As part of the error handling refactoring of #3261, we should
not be setting a custom status code for validation routes.
We should rely only on the error.statusCode property instead and
leave the user full customization capabilities. Unfortunately
a change was missed.

Fixes: #4048
Ref: #3261
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jsumners jsumners jsumners left review comments

@kibertoad kibertoad kibertoad left review comments

@zekth zekth zekth approved these changes

@sergejostir sergejostir sergejostir left review comments

@fox1t fox1t fox1t approved these changes

@delvedor delvedor delvedor approved these changes

@Eomm Eomm Eomm approved these changes

Assignees
No one assigned
Labels
semver-major Issue or PR that should land as semver major
Projects
None yet
Milestone
v4.0.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@mcollina @sergejostir @jsumners @fox1t @kibertoad @delvedor @zekth @Eomm