fix(#5220): better support encapsulated synchronous error handlers #5222

domdomegg · 2023-12-20T05:15:53Z

Parent error handling for synchronous error handlers was largely a unintended byproduct of some other try catches previously.

This could result in a server crash if multiple error handlers were used, and rethrew errors (see #5220).

This PR fixes this behavior by retrying synchronous error handling (similar to how we already do in wrapThenable for asynchronous error handling). It also adds regression tests to ensure this continues to work properly.

Checklist

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

…lers Parent error handling for synchronous error handlers was largely a unintended byproduct of some other try catches previously. This could result in a server crash if multiple error handlers were used, and rethrew errors (see fastify#5220). This PR fixes this behavior by retrying synchronous error handling (similar to how we already do in wrapThenable for asynchronous error handling). It also adds regression tests to ensure this continues to work properly.

mcollina

Good job. Only one consideration.

lib/error-handler.js

test/encapsulated-error-handler.test.js

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

lib/error-handler.js

metcoder95 · 2023-12-24T16:18:46Z

lib/error-handler.js

+      if (err.cause) {
+        throw new Error('Child error already has a cause set')
+      }


Curious, should we throw?

What are the expectations here, if let's say, I throw a custom Error that already has a cause property already set?

Feels unintuitive to only sometimes edit the cause property. But overwriting it and losing data sometimes seems worse.

I initially suggested we add it as a symbol property, but cause was preferred: #5222 (comment)

metcoder95

Tests seems failing

metcoder95 · 2024-01-14T14:21:03Z

lib/error-handler.js

+      if (err.cause) {
+        throw new Error('Child error already has a cause set')
+      }


I'm over the fence on this one; that can cause more confusion when dealing with unexpected errors.

In the scenario that I have a situation like this, where my err.cause already is set, I might prefer to have another wrapper on top that preserves the stack trace so I can fully track down the issue instead of throwing a new error that swallows the original one.

Maybe we can find a better strategy, but not sure throwing and swallowing might be the best way to go

Can you clarify what you think is swallowing the error? It'd be the custom error handler implementation that could overwrite the cause - otherwise users would get the full stack trace by default.

metcoder95

Tests seems failing

domdomegg · 2024-01-14T15:27:47Z

It feels like we keep going back and forth over what fastify should overwrite and not overwrite here, and what should be logged. This seems secondary to fixing what seems more important: that the current error handling behaviour can cause a fatal crash when multiple error handlers are used.

If you have specific preferences in mind, could you state how you'd like this to be implemented? Because at the moment it feels like at each review we tweak it a little but the PR is actually getting worse and no closer to being merged.

I don't think it's worth continuing work on this until it's clear what the maintainers would prefer.

metcoder95 · 2024-01-15T17:00:13Z

Good call! First of all, thanks for taking the time on this.
Second, I partially disagree with you as the expectation is clear, but we are digging too much into the details.

The PR is halfway done, so let's clarify some points:

If we encounter the .cause is already set, what do we do?
- Problably the best we can do is to log it to keep it simple.
If setting the .cause fails because the object is frozen or any other reason, what do we do?
- Let's just log it for now, so implementers are aware.

What are your thoughts?
cc: @mcollina

domdomegg · 2024-01-16T01:23:14Z

Thank you for your quick response. I'm happy to implement it the way you've laid out. However, I'm a little stuck as I think the thing you've written out there is what has already been implemented. There are also tests for the cases you've suggested (.cause set should log, .cause fails because it's frozen should log).

I'm therefore not sure what the other half of the PR is you are looking for ("The PR is halfway done"), or where the comment threads we've been having are going. Could you clarify further on this, because I think this is where I am probably most confused!

Also sorry if I'm being dumb here and missing something obvious - do please restate it if you think so! I acknowledge you think the expectations are clear, but as a first-time contributor to fastify I'm still a little lost 😅. I appreciate your support in reviewing this PR and helping clarify things thus far!

Because of how error handlers wrap things, following the control flow in the tets can be tricky. In this test file I've added numbered comments indicate the order statements are expected to execute. I've also standardised the names of things being thrown so it's easier to follow them.

MattJustMatt · 2024-01-25T07:59:00Z

I wanted to chime in as a user hitting this issue in prod- we noticed that throwing from one of our error handlers was crashing the Fastify process. We switched to manually log the issue but losing out on automated fastify-sentry alerting etc... of course we can add this in the manual handling of the error, but if this PR solves the issue I'd like to help.

If there are any eyes or extra hands needed for this PR please let me know if I can help.

mcollina

Good work, one tiny bit and it's good to go

lib/error-handler.js

mcollina

lgtm

Signed-off-by: Aras Abbasi <aras.abbasi@googlemail.com>

Uzlopak · 2024-01-30T12:32:22Z

docs/Reference/Errors.md

@@ -322,6 +323,7 @@ but the body is being consumed. | Make sure you don't consume the `Response.body
 | <a id="fst_err_bad_status_code">FST_ERR_BAD_STATUS_CODE</a> | The status code is not valid. | Use a valid status code. | [#2082](https://github.com/fastify/fastify/pull/2082) |
 | <a id="fst_err_bad_trailer_name">FST_ERR_BAD_TRAILER_NAME</a> | Called `reply.trailer` with an invalid header name. | Use a valid header name. | [#3794](https://github.com/fastify/fastify/pull/3794) |
 | <a id="fst_err_bad_trailer_value">FST_ERR_BAD_TRAILER_VALUE</a> | Called `reply.trailer` with an invalid type. Expected a function. | Use a function. | [#3794](https://github.com/fastify/fastify/pull/3794) |
+| <a id="fst_err_error_cause_already_present">FST_ERR_ERROR_CAUSE_ALREADY_PRESENT</a> | An error handler threw an error with the `cause` property set, so Fastify will not set the cause to the original error. | If used, ensure the cause property is as expected. | [#5222](https://github.com/fastify/fastify/pull/5222) |


"If used, ensure the cause property is as expected." sounds so strange.

I mean this is the remedy to solve this error. But reading this, I would not know what to do. Can you please elaborate?

@Fdawgs
Can you please give your opinion?

Suggested change

| <a id="fst_err_error_cause_already_present">FST_ERR_ERROR_CAUSE_ALREADY_PRESENT</a> | An error handler threw an error with the `cause` property set, so Fastify will not set the cause to the original error. | If used, ensure the cause property is as expected. | [#5222](https://github.com/fastify/fastify/pull/5222) |

| <a id="fst_err_error_cause_already_present">FST_ERR_ERROR_CAUSE_ALREADY_PRESENT</a> | An error handler threw an error with the `cause` property set, so Fastify will not set `cause` to the original error. | If used, ensure the `cause` property is as expected. | [#5222](https://github.com/fastify/fastify/pull/5222) |

@Uzlopak I think 'cause' needs to be wrapped in backticks consistently. Otherwise 'cause' would be read as a verb rather than a noun, which I think is why it reads strangely currently.

Yeah, but what is the remedy? You get this error, and you should check if it i what you expected? How does this help?

Ah gotcha, I see what you mean. Needs a think.

jsumners · 2024-01-31T01:19:08Z

I'll try to review tomorrow.

Uzlopak

Having some doubts... Will elaborate them in few hours.

jsumners · 2024-01-31T13:47:07Z

lib/error-handler.js

+    try {
+      if (err.cause) {
+        throw FST_ERR_ERROR_CAUSE_ALREADY_PRESENT()
+      }
+      err.cause = error
+    } catch (failedToAssignError) {
+      reply.log.warn({
+        err: error,
+        parentError: err,
+        reason: failedToAssignError,
+        message: 'Failed to assign child error as cause to parent error thrown from error handler'
+      })


It's not clear to me why we don't:

if (Object.prototype.hasOwnProperty.call(err, 'cause') === true) { err = Error('something or other', { cause: error }) } else { err.cause = error }

Instead of a second try/catch and adding a log people are going to miss (and likely won't want, resulting in new issues being filed when they do notice it).

@jsumners

His claim is, that if Error Object is frozen or differently protected, you can not set the attribute.

I would not be concerned about encountering a frozen error instance. That is extremely unlikely to happen, and probably a bug if it does.

That is extremely unlikely to happen, and probably a bug if it does.

The whole point of this function is to handle errors, i.e. cases where things aren't going as expected. I'd argue this is exactly the place to be robust to edge cases, especially because if we do throw an error here it crashes the entire server.

Can you provide an example of anything generating and throwing a frozen error object?

Uzlopak · 2024-01-31T14:18:41Z

lib/error-handler.js

+    reply[kReplyIsError] = true
+    try {
+      if (err.cause) {
+        throw FST_ERR_ERROR_CAUSE_ALREADY_PRESENT()


We throw an Error, to abort the control flow. But we instantiate an Error and generate a stacktrace, which we probably dont need anyway?
We dont need it to be in the try catch in this case.

Are you suggesting we could do this instead if err.cause is already set, without entering the try/catch?:

reply.log.warn({ err: error parentError: err, reason: FST_ERR_ERROR_CAUSE_ALREADY_PRESENT(), message: 'Failed to assign child error as cause to parent error thrown from error handler' })

If so, what would the benefit be here?

Uzlopak · 2024-02-08T10:37:18Z

I think this feature is needed, but the implementation "feels" not "right".

Uzlopak · 2024-02-21T00:56:21Z

I think following thing needs to be modified:

In lib/error-handler.js instead of wrapping it into a second try catch block just to be sure we can assign a .cause attribute to the error we create a new FST_ERR_FAILED_ERROR (name pending) and assign a .cause to this always. Avoids to use a second try catch.

We then dont even need to warn of a failed attempt to set cause because we could not set .cause of the error

The rest was not disputed.

domdomegg · 2024-05-08T00:21:24Z

Closing as effectively the same fix as the original commit has been done in #5445

mcollina requested changes Dec 20, 2023

View reviewed changes

lib/error-handler.js Show resolved Hide resolved

metcoder95 requested changes Dec 20, 2023

View reviewed changes

test/encapsulated-error-handler.test.js Outdated Show resolved Hide resolved

test/encapsulated-error-handler.test.js Outdated Show resolved Hide resolved

domdomegg and others added 2 commits December 20, 2023 10:38

Apply suggestions from code review

61eee8f

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

Remove now defunct extra catch in wrapThenable

b40ea92

domdomegg mentioned this pull request Dec 20, 2023

[FEATURE] --fail-only tapjs/tapjs#983

Open

2 tasks

Merge branch 'main' into fix-5220

6325ca6

Eomm added the bugfix Issue or PR that should land as semver patch label Dec 23, 2023

domdomegg added 4 commits December 23, 2023 13:09

Address review comments

4b6e2dd

Add test to make coverage checker happy

c8a79af

Make node 14 tests happy

471307c

Fix test for node 14

68ee1d3

domdomegg requested review from metcoder95 and mcollina December 23, 2023 14:23

mcollina requested changes Dec 23, 2023

View reviewed changes

lib/error-handler.js Show resolved Hide resolved

lib/error-handler.js Outdated Show resolved Hide resolved

metcoder95 reviewed Dec 24, 2023

View reviewed changes

domdomegg added 2 commits January 14, 2024 13:11

Update log line as per review comment

11f1afb

Merge branch 'main' into fix-5220

dd61c69

metcoder95 reviewed Jan 14, 2024

View reviewed changes

domdomegg added 2 commits January 16, 2024 01:57

Merge branch 'main' into fix-5220

2b820f3

mcollina reviewed Jan 25, 2024

View reviewed changes

lib/error-handler.js Outdated Show resolved Hide resolved

domdomegg added 2 commits January 28, 2024 16:54

Merge branch 'main' into fix-5220

2483fd6

Add Fastify error

456397f

mcollina approved these changes Jan 28, 2024

View reviewed changes

mcollina requested review from jsumners, Uzlopak and Eomm January 28, 2024 18:15

metcoder95 approved these changes Jan 28, 2024

View reviewed changes

Uzlopak added 2 commits January 30, 2024 13:19

Merge branch 'main' into fix-5220

3440874

Signed-off-by: Aras Abbasi <aras.abbasi@googlemail.com>

fix number of errors

f27b58d

Uzlopak reviewed Jan 30, 2024

View reviewed changes

Uzlopak requested a review from Fdawgs January 30, 2024 17:22

Uzlopak added the citgm-core-plugins label Jan 30, 2024

Uzlopak removed the citgm-core-plugins label Jan 31, 2024

Uzlopak requested changes Jan 31, 2024

View reviewed changes

jsumners reviewed Jan 31, 2024

View reviewed changes

Uzlopak reviewed Jan 31, 2024

View reviewed changes

domdomegg closed this May 8, 2024

domdomegg mentioned this pull request May 8, 2024

Add tests for error handling #5451

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#5220): better support encapsulated synchronous error handlers #5222

fix(#5220): better support encapsulated synchronous error handlers #5222

domdomegg commented Dec 20, 2023 •

edited

mcollina left a comment

metcoder95 Dec 24, 2023

domdomegg Jan 14, 2024

metcoder95 left a comment

metcoder95 Jan 14, 2024

domdomegg Jan 14, 2024

metcoder95 left a comment

domdomegg commented Jan 14, 2024

metcoder95 commented Jan 15, 2024

domdomegg commented Jan 16, 2024

MattJustMatt commented Jan 25, 2024

mcollina left a comment

mcollina left a comment

Uzlopak Jan 30, 2024 •

edited

Uzlopak Jan 30, 2024

Fdawgs Jan 30, 2024 •

edited

Uzlopak Jan 31, 2024

Fdawgs Jan 31, 2024

jsumners commented Jan 31, 2024

Uzlopak left a comment

jsumners Jan 31, 2024

Uzlopak Jan 31, 2024

jsumners Jan 31, 2024

domdomegg Feb 3, 2024

jsumners Feb 3, 2024

Uzlopak Jan 31, 2024

domdomegg Feb 3, 2024 •

edited

Uzlopak commented Feb 8, 2024

Uzlopak commented Feb 21, 2024

domdomegg commented May 8, 2024

	\| <a id="fst_err_error_cause_already_present">FST_ERR_ERROR_CAUSE_ALREADY_PRESENT</a> \| An error handler threw an error with the `cause` property set, so Fastify will not set the cause to the original error. \| If used, ensure the cause property is as expected. \| [#5222](https://github.com/fastify/fastify/pull/5222) \|
	\| <a id="fst_err_error_cause_already_present">FST_ERR_ERROR_CAUSE_ALREADY_PRESENT</a> \| An error handler threw an error with the `cause` property set, so Fastify will not set `cause` to the original error. \| If used, ensure the `cause` property is as expected. \| [#5222](https://github.com/fastify/fastify/pull/5222) \|

fix(#5220): better support encapsulated synchronous error handlers #5222

fix(#5220): better support encapsulated synchronous error handlers #5222

Conversation

domdomegg commented Dec 20, 2023 • edited

Checklist

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metcoder95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metcoder95 left a comment

Choose a reason for hiding this comment

domdomegg commented Jan 14, 2024

metcoder95 commented Jan 15, 2024

domdomegg commented Jan 16, 2024

MattJustMatt commented Jan 25, 2024

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Uzlopak Jan 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fdawgs Jan 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsumners commented Jan 31, 2024

Uzlopak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domdomegg Feb 3, 2024 • edited

Choose a reason for hiding this comment

Uzlopak commented Feb 8, 2024

Uzlopak commented Feb 21, 2024

domdomegg commented May 8, 2024

domdomegg commented Dec 20, 2023 •

edited

Uzlopak Jan 30, 2024 •

edited

Fdawgs Jan 30, 2024 •

edited

domdomegg Feb 3, 2024 •

edited