fix(#5049): Remove duplicated calls to onReady by metcoder95 · Pull Request #5051 · fastify/fastify

metcoder95 · 2023-09-20T08:13:50Z

This PR closes #5049

It fixes the double run of onReady hooks when calling Fastify#ready more than once.
It introduces two new states booting (used to indicate onReady hooks are running) and ready (used to indicate onReady hooks have finished running).

These additions can be subject to change, but I believe they are a good starting point.

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

mcollina

I think this implementation is incorrect. While the second call to ready() would not execute the hooks, it will resolve before the first one.

Uzlopak · 2023-09-20T08:51:11Z

Something like this has to be utilized.
https://github.com/fastify/avvio/blob/master/lib/create-promise.js

metcoder95 · 2023-09-20T09:03:38Z

Yeah, got you. I believe I rushed the implementation a little but wanted to keep a starting point as it was a little bit more complex than I expected.

My initial thought was to keep the promise somewhere, so the same promise could be returned and have deterministic results.

e.g.

function getPromise () {
  let res, rej
  const promise = new Promise((resolve, reject) => { res = resolve; rej = reject })

  return { promise, resolve: res, reject: rej }
}

function ready (cb) {
  const { promise, resolve: resolveReady, reject: rejectReady } = fastify[kState].promise = fastify[kState].promise == null
    ? getPromise()
    : fastify[kState].promise

  // run the hooks after returning the promise
  !fastify[kState].booting && !fastify[kState].ready && process.nextTick(runHooks)

  if (!cb) {
    return promise
  }

	// rest of implementation
}

The previous example resolves as expected:

const app = Fastify()
let counter = 0

app.addHook('onReady', async function () {

})

await Promise.race([
  app.ready().then(() => console.log('1')),
  app.ready().then(() => console.log('2')),
  app.ready().then(() => console.log('3')),
  app.ready().then(() => console.log('4'))
])

await app.ready()
console.log('5')

Results in:

test/hooks.on-ready.test.js 1> 1
test/hooks.on-ready.test.js 1> 2
test/hooks.on-ready.test.js 1> 3
test/hooks.on-ready.test.js 1> 4
test/hooks.on-ready.test.js 1> 5

metcoder95 · 2023-09-20T09:10:09Z

For callbacks, things get more complex, maybe if the app is already booting we keep a record of them and call them sequentially?

The simplest way is just to throw an error that the app is already starting (or even a silent warning). Thoughts?

Uzlopak · 2023-09-20T09:15:56Z

no, with that create promise from avvio you can handle all cases properly.

mcollina · 2023-09-20T09:16:14Z

Oh I know this is was a tricky problem, that's why I opened a bug instead of sending a quick PR! It's also quite an edge case.

For callbacks, things get more complex, maybe if the app is already booting we keep a record of them and call them sequentially?

This sounds very tricky. Maybe using an EventEmitter somewhere that spins events at the change of states? So if it's booting, you can wait for the "ready" event instead.

The simplest way is just to throw an error that the app is already starting (or even a silent warning). Thoughts?

This is worse than our current bug :(.

metcoder95 · 2023-09-20T18:42:47Z

no, with that create promise from avvio you can handle all cases properly.

Can you elaborate @Uzlopak maybe I'm not getting your suggestion right 🤔

Oh I know this is was a tricky problem, that's why I opened a bug instead of sending a quick PR! It's also quite an edge case.

Haha, I realized until I opened the PR 😅

Maybe using an EventEmitter somewhere that spins events at the change of states? So if it's booting, you can wait for the "ready" event instead.

Hmm, that sounds interesting. We can keep it tight to the instance and just subscribe new calls to it. I have a rough idea of a similar thing but with the Promise itself; I'll give it a shot to ;)

This is worse than our current bug :(.

Yeah, don't like it either but it was also an option 😅

metcoder95 · 2023-09-21T07:54:22Z

Ah ok okay, I see; it is similar to what I was trying to test out. You gave me an idea though, I'll set back the PR into draft and re-work the approach 👍

metcoder95 · 2023-11-12T16:38:39Z

I've refactored it trying to attempt to chain incoming promises down to the first one ever created when initially calling ready. That solves mixed implementations from callbacks and promises, as well as each one independently, but it does not avoids the call to the avvio hook loader.

We can go in this direction if we think that not avoiding the avvio loader twice is a problem.

fastify.js

test/fastify-instance.test.js

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

metcoder95 · 2023-11-19T12:35:41Z

Do we have an issue with warning?

metcoder95 · 2023-11-19T12:36:44Z

Tried to enhance sequence handling of the CB and Promises by creating a Promise upfront and using it as a flow control mechanism. Further CB's are attached to the callbacks seeking for ordering.

Eomm · 2023-11-19T12:40:15Z

Do we have an issue with warning?

We released a fix this morning of the process-warning module. Does it brake the test?
I'm with the mobile 📱 and can't check it right now

metcoder95 · 2023-11-19T12:43:00Z

It seems so, I rebased with main and tests are failing due to:
warming.emitted.<set | delete | ..> are not a function.

https://github.com/fastify/fastify/actions/runs/6920346527/job/18824804630?pr=5051#step:5:2881

github-actions · 2023-12-10T17:03:14Z

Node: 16
PR: [1] 1721k requests in 30.04s, 323 MB read
MAIN: [1] 1786k requests in 30.04s, 336 MB read

Node: 18
PR: [1] 1447k requests in 30.03s, 272 MB read
MAIN: [1] 1463k requests in 30.04s, 275 MB read

Node: 20
PR: [1] 1515k requests in 30.04s, 285 MB read
MAIN: [1] 1526k requests in 30.04s, 287 MB read

Uzlopak

RSLGTM

Uzlopak

Looks right

So no RSLGTM but a LGTM

Uzlopak · 2023-12-13T02:30:42Z

@mcollina
PTAL

mcollina

lgtm

github-actions · 2024-12-14T00:29:20Z

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.

metcoder95 requested a review from mcollina September 20, 2023 08:14

mcollina requested changes Sep 20, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

metcoder95 marked this pull request as draft September 21, 2023 07:54

metcoder95 force-pushed the fix/5049 branch from a0c3062 to 5eedfc5 Compare November 12, 2023 12:29

metcoder95 marked this pull request as ready for review November 12, 2023 16:22

metcoder95 marked this pull request as draft November 12, 2023 16:36

metcoder95 marked this pull request as ready for review November 12, 2023 16:38

metcoder95 requested a review from mcollina November 12, 2023 16:38

Eomm reviewed Nov 13, 2023

View reviewed changes

fastify.js Outdated Show resolved Hide resolved

fastify.js Outdated Show resolved Hide resolved

test/fastify-instance.test.js Show resolved Hide resolved

metcoder95 and others added 4 commits November 19, 2023 13:07

fix: call onready once

207cf50

fix: ensure on-ready promises are triggered in the right order

1f24b6e

refactor: Apply suggestions from code review

de40515

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

refactor: use promise as flow management

447beeb

metcoder95 force-pushed the fix/5049 branch from 028d76b to 447beeb Compare November 19, 2023 12:07

metcoder95 requested a review from Eomm November 19, 2023 12:35

Eomm mentioned this pull request Nov 20, 2023

fix: emitter api fastify/process-warning#91

Closed

Merge branch 'main' into fix/5049

696609f

Merge branch 'main' into fix/5049

b0117a3

Uzlopak added the benchmark Label to run benchmark against PR and main branch label Dec 10, 2023

github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Dec 10, 2023

Uzlopak approved these changes Dec 10, 2023

View reviewed changes

Merge branch 'main' into fix/5049

5f49cf3

Uzlopak approved these changes Dec 12, 2023

View reviewed changes

Merge branch 'main' into fix/5049

e499cff

mcollina approved these changes Dec 13, 2023

View reviewed changes

Uzlopak merged commit 1b588a0 into main Dec 13, 2023

Uzlopak deleted the fix/5049 branch December 13, 2023 07:02

renovate bot mentioned this pull request Jan 9, 2024

fix(deps): update all non-major dependencies jellydn/nft-app#1070

Closed

1 task

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

Uh oh!

Conversation

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

Uh oh!

Checklist

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Sep 20, 2023

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uzlopak commented Sep 20, 2023

Uh oh!

mcollina commented Sep 20, 2023

Uh oh!

metcoder95 commented Sep 20, 2023

Uh oh!

This comment was marked as outdated.

metcoder95 commented Sep 21, 2023

Uh oh!

metcoder95 commented Nov 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metcoder95 commented Nov 19, 2023

Uh oh!

metcoder95 commented Nov 19, 2023

Uh oh!

Eomm commented Nov 19, 2023

Uh oh!

metcoder95 commented Nov 19, 2023

Uh oh!

github-actions bot commented Dec 10, 2023

Uh oh!

Uzlopak left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Dec 13, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

metcoder95 commented Sep 20, 2023 •

edited

Loading

metcoder95 commented Sep 20, 2023 •

edited

Loading

metcoder95 commented Sep 20, 2023 •

edited

Loading