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.

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

Error handler settings fails when preceded by an awaited register #3863

Closed
2 tasks done
omothm opened this issue Apr 29, 2022 · 9 comments
Closed
2 tasks done

Error handler settings fails when preceded by an awaited register #3863

omothm opened this issue Apr 29, 2022 · 9 comments
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor

Comments

@omothm
Copy link
Contributor

omothm commented Apr 29, 2022

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

3.29.0

Plugin version

No response

Node.js version

16.15.0

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

12.3.1

Description

When I set an error handler on a Fastify instance after registering a plugin within an awaited statement, the error handler does not work as expected.

An example scenario: Let f be a Fastify instance.

  • Add a / get handler on instance f that throws an error.
  • Register a plugin on instance f with any prefix (e.g., /bar) with an await keyword.
  • Set error handler (call it h) on instance f.

When fetching / on this instance, an error is thrown and h is expected to run. However, it does not. The default handler is run instead.

Removing the await keyword fixes this issue. But the existence of the await keyword or the nonexistence thereof should not alter any behavior--unless there is an intricacy in this situation that forces such behavior.

Steps to Reproduce

A reproduction of the issue can be found here:
https://github.com/omothm/fastify-error-handler-await-repro

Expected Behavior

All 3 test scenarios in the reproduction repo are expected to pass. Currently, one scenario fails (the one with await instance.register()) but was expected to pass.

@mcollina
Copy link
Member

This is... expected given some of the intricacies in the code. However I think this is solved in v4.0.0-rc.1. Can you check?

@omothm
Copy link
Contributor Author

omothm commented Apr 30, 2022

I've checked and, unfortunately, the behavior is the same in v4.0.0-rc.1.

This bug has cost me 2 days to figure out. I know it's on me, not the framework. But I thought pointing out this issue might help other developers.

I can't think of robust workarounds to make this unexpected behavior clear to the user. Perhaps the framework typing should remove the PromiseLike return value of instance.register()? This way, mistakenly awaiting the function would make the IDE throw a warning. (If there are use cases that require the PromiseLike interface, then this suggestion is obviously impractical.)

Or maybe put somewhere in the docs a concrete description of what to expect by awaiting/non-awaiting in conjunction with the effect of calls order (because awaiting after setting the error handler actually works)?

@climba03003
Copy link
Member

climba03003 commented May 1, 2022

The await for .register is very useful. It can reduce the code like.

// without await
fastify.register(something).after(something)

// with await 
await fastify.register(something)
// ...something depends on top

The problem of your issue is that, when you await the plugin. It will finalize the encapsulation process, so all the mutation on the parent instance will not be inherited.

IMHO, I only recommend to await the plugin which wrapped by fastify-plugin. In this case, it will not have the side-effect like you mention.

@omothm
Copy link
Contributor Author

omothm commented May 3, 2022

@climba03003 Thank you for your elaboration. How can we make it clear to the developer that awaiting a plugin finalizes encapsulation (and the consequences thereof)?

Perhaps adding a sentence or two here could prove useful. Do you have any recommended wording for the behavior that could be added there?

@mcollina mcollina added enhancement documentation Improvements or additions to documentation good first issue Good for newcomers labels May 4, 2022
@mannyistyping
Copy link

Hey there all 👋🏽
Long time user, first time contributor.

May I take this "good first issue" on?

My plan would be to put in a PR to update plugins.md#asyncawait with the guidance that @climba03003 shared above.

@mcollina
Copy link
Member

go for it!

@Eomm Eomm added semver-minor Issue or PR that should land as semver minor and removed enhancement labels Apr 1, 2023
@AliakbarETH
Copy link
Contributor

Hi @mcollina , can you assign me this one?

I would love to work on it.

Regards

@climba03003
Copy link
Member

can you assign me this one?

There is no need for assignment.
If you would like to works on it. Just send the PR.

@Eomm
Copy link
Member

Eomm commented Aug 26, 2023

Closed by #4846

@Eomm Eomm closed this as completed Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

No branches or pull requests

7 participants
@mcollina @Eomm @climba03003 @AliakbarETH @omothm @mannyistyping and others