fix: ensure the startup is completed before calling `startupCompleted` #7525

sadortun · 2021-08-27T01:44:44Z

New Pull Request Checklist

I am not disclosing a vulnerability.
I am creating this PR in reference to an issue.

Issue Description

During #7418, i noticed that migrations were not always executed before the cloud were loaded, or serverStartComplete(), which in some case cause problem, because ParseCloud is loaded before migrations had time to complete, which can be quite bad !

This PR aims to ensure a proper sequential startup. In #7418 i will add Schema Migrations between hooksController.load() and serverStartComplete()

I've spitted this here so if we need to discuss this topic on its own.

Related issue: #7527 #7418

Approach

TODOs before merging

Add test cases
Add entry to changelog

mtrezza · 2021-08-27T14:30:50Z

@sadortun Thanks for discovering this bug and opening this PR!

Can you create a separate issue for this fix? #7418 is a feature request, but this seems to be a bug fix, so a separate issue would he helpful.

dblythy · 2021-09-03T07:39:24Z

Is this ready for review @sadortun?

sadortun · 2021-09-03T13:03:52Z

@dblythy only missing thing is to move serverStartComplete after cloud init.

I'll try to finish this and the few other open PR for next Monday.

sadortun · 2021-09-03T13:10:24Z

@mtrezza @dblythy do the security checks need to run after anything is initialized ? Or they can be run independently?

Before, they were ran completly independently of the db/cloud init

mtrezza · 2021-09-03T13:12:53Z

do the security checks need to run after anything is initialized

yes

sadortun · 2021-09-03T13:59:35Z

Currently the Security checks can run before everything is initialized.

Should I fix this here, or in a separate issue ?

For the tests to succeed, we will need a way to know the init has completed. Aka #7527

IMO, it would be best to fix/refactor the constructor init issues in one issue (#7527 and this PR) since it's all related.

mtrezza · 2021-09-03T14:04:13Z

Currently the Security checks can run before everything is initialized.

Are you referring to this?

parse-server/src/ParseServer.js

Lines 112 to 114 in 9ea355b

    
           if (security && security.enableCheck && security.enableCheckLog) { 
        
             new CheckRunner(options.security).run(); 
        
           }

mtrezza · 2021-09-03T14:17:29Z

Should I fix this here, or in a separate issue ?

It's a separate issue, if it is currently an issue at all.

IMO, it would be best to fix/refactor the constructor init issues in one issue (#7527 and this PR) since it's all related.

We cannot backport breaking changes for code that is outside the scope of a vulnerability. If the constructor change is breaking, but it is not required to fix this bug, it cannot be included in the bug fix. If the constructor change is not breaking, and it reduces complexity to change the constructor together with fixing the bug because they are depending on each other, then they can be done in one PR, because we can backport them.

sadortun · 2021-09-03T15:12:37Z

Currently the Security checks can run before everything is initialized.

Are you referring to this?

parse-server/src/ParseServer.js

Lines 112 to 114 in 9ea355b

if (security && security.enableCheck && security.enableCheckLog) {

new CheckRunner(options.security).run();

}

Yes.

sadortun · 2021-09-03T15:18:55Z

We cannot backport breaking changes for code that is outside the scope of a vulnerability. If the constructor change is breaking, but it is not required to fix this bug, it cannot be included in the bug fix.

I really don't know if it's an issue or not, tests currently only check that the CheckRunner.run() are executed. But they can be executed before the init has completed.

I'm not going to touch it for now, and they'll keep being executed even if the init is not completed.

If it's an issue/bug/vulnerability, please open an issue.

Also the CheckRunner.run() itself is an async method, but it it's not awaited for.

mtrezza · 2021-09-03T15:26:17Z

I really don't know if it's an issue or not, tests currently only check that the Security check are executed. But they can be executed before the init has completed.

Depends on the checks, intuitively I would say that they should be executed after initialization, but since the tests all pass, it shouldn't be an issue now. As long as the logger is loaded, I believe the checks are covered. When adding new checks, it may become necessary to address this.

Also the CheckRunner.run() is an adync method, but it it's not awaited for.

It doesn't need await (and slow down server init), it just prints out logs.

sadortun · 2021-09-03T15:29:37Z

Ok, perfect!

I'll update this PR to reflect what we discussed above and I'll let you know once it's done.

mtrezza · 2021-09-03T15:32:29Z

Amazing!

sadortun · 2021-09-03T16:16:59Z

✅ Updated.

@mtrezza @dblythy Its ready for review.

codecov · 2021-09-03T16:21:05Z

Codecov Report

Merging #7525 (0e6ecba) into alpha (b64640c) will increase coverage by 0.00%.
The diff coverage is 83.33%.

@@           Coverage Diff           @@
##            alpha    #7525   +/-   ##
=======================================
  Coverage   93.88%   93.88%           
=======================================
  Files         183      183           
  Lines       13624    13625    +1     
=======================================
+ Hits        12791    12792    +1     
  Misses        833      833

Impacted Files	Coverage Δ
src/ParseServer.js	`90.10% <83.33%> (+0.05%)`	⬆️
src/Adapters/Files/GridFSBucketAdapter.js	`79.50% <0.00%> (-0.82%)`	⬇️
src/RestWrite.js	`94.04% <0.00%> (ø)`
src/batch.js	`93.10% <0.00%> (+1.72%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d35eeb8...0e6ecba. Read the comment docs.

mtrezza · 2021-09-03T16:24:33Z

Great, could you add a changelog entry and update the TODOs at the top?

src/ParseServer.js

sadortun · 2021-09-03T16:48:09Z

Great, could you add a changelog entry and update the TODOs at the top?

Done.

src/ParseServer.js

mtrezza · 2021-09-09T12:29:57Z

Are there any open questions that need to be resolved?

sadortun · 2021-09-09T16:28:09Z

Are there any open questions that need to be resolved?

The last discussion above, I'll try to finish It today, but I'm drowning in other stuff 🌊

mtrezza · 2021-09-09T23:03:42Z

@sadortun Thanks, no rush, just wanted to check what the status is. Maybe @dblythy has a chance to respond to your last comment meanwhile.

sadortun · 2021-10-05T06:59:21Z

[ .... one eternity later .... ]

Here you go !

✅ Resolved discussions
✅ Rebased

mtrezza · 2021-10-08T15:40:35Z

Is this ready for review? @dblythy could you take a look?
@sadortun would you be available to do PR reviews at times? If so, I would add you to the server review team that gets notifications when PRs are ready for review.

sadortun · 2021-10-08T17:14:50Z

would you be available to do PR reviews at times

Yeah sure !

parse-github-assistant · 2021-10-09T00:36:12Z

Thanks for opening this pull request!

🎉 We are excited about your hands-on contribution!

mtrezza · 2021-10-09T00:37:47Z

@sadortun is there any way we can add tests for this? that should also solve the coverage decrease.

dblythy

Looks mostly good couple of minor changes.

I have addressed the coverage issues in my cloud code PR (#7564)

src/ParseServer.js

mtrezza · 2021-10-13T19:58:08Z

What do we need to merge this?

Moumouls · 2021-10-14T06:54:00Z

Here @sadortun i think you only need to move

The cloud code loading is sync, then we are safe to execute init

  if (cloud) {
      addParseCloud();
      if (typeof cloud === 'function') {
        cloud(Parse);
      } else if (typeof cloud === 'string') {
        if (process.env.npm_package_type === 'module') {
          import(path.resolve(process.cwd(), cloud));
        } else {
          require(path.resolve(process.cwd(), cloud));
        }
      } else {
        throw "argument 'cloud' must either be a string or a function";
      }
    }

before

 databaseController
      .performInitialization()
      .then(() => hooksController.load())
      .then(async () => {
        if (schema) {
          await new DefinedSchemas(schema, this.config).execute();
        }
        if (serverStartComplete) {
          serverStartComplete();
        }
      })
      .catch(error => {
        if (serverStartComplete) {
          serverStartComplete(error);
        } else {
          console.error(error);
          process.exit(1);
        }
      });

I'll also take a deeper look to the init on the defined schema PR

EDIT:

Since on defined schema PR we have

.then(async () => {
        if (schema) {
          await new DefinedSchemas(schema, this.config).execute();
        }
        if (serverStartComplete) {
          serverStartComplete();
        }
      })

Here just moving up the cloud code register should be okay. Make sense if a user wants to trigger a cloud code function in the beforeMigration function.

@sadortun i think you can easily write a failing test, trying to use a cloud function into the beforeMigration function once the defined schema PR is merged.

Also since this issue is related to the defined schema feature, here i suggest to close this PR and add directly the fix + test on the defined schema PR. @mtrezza @dblythy @sadortun what do you think

Moumouls · 2021-10-14T07:10:44Z

Also here the correct structure to ensure proper startup of Parse Server.

const server = new Promise((resolve) => {
const parseServer =  ParseServer.start({ serverStartComplete: () => {
  resolve(parseServer)
// Add a dedicated health check endpoint if you want
  parseServer.expressApp.get('/ready', (req: any, res: any) => {
	res.send('true')
  })
  })
})
await server
// Server is loaded
console.log('Started')

@sadortun exemple code herer https://github.com/Moumouls/next-atomic-gql-server/blob/master/src/server.ts

May be here you are right about returning a promise for ParseServer.start, it will be easy to await server start.

const server = await ParseServer.start()
console.log('Server started')

Refactoring the parse server start to async/await will solve the issue and aslo allow to remove the old .then syntax

mtrezza · 2021-11-06T13:39:19Z

Note this this PR (like any other) must not contain any breaking changes without deprecation, see #7680 (comment).

mtrezza · 2021-11-06T14:31:33Z

@sadortun could you merge alpha into this and resolve any conflicts?

@Moumouls could you give this a quick review? you mentioned some feedback regarding this yesterday. and maybe we can close (or continue) the open questions above.

Moumouls

Hi @sadortun @dblythy @mtrezza

After a after a careful reading, this PR will introduce a new race issue. We have a similar issue like the import.

Currently ParseServer was always started in a sync way. Cloud code is also loaded in sync.

Here the PR start to load the code (even with require) in async since require is now in a then function.

So this PR introduce a race and also a breaking change if some developers do not use serverStartComplete to correctly await the full start.

Here we cannot solve the import race issue. Also to keep this PR focused on the original issue, i'll recommend to just move the old

    if (cloud) {
      addParseCloud();
      if (typeof cloud === 'function') {
        cloud(Parse);
      } else if (typeof cloud === 'string') {
        if (process.env.npm_package_type === 'module') {
          import(path.resolve(process.cwd(), cloud));
        } else {
          require(path.resolve(process.cwd(), cloud));
        }
      } else {
        throw "argument 'cloud' must either be a string or a function";
      }
    }

Before

databaseController
      .performInitialization()

Then ParseServer will load correctly in this order:

load cloud code (sync with require, same JS loop as the Parse Server constructor)
start db init
after db init load hooks
after load hooks load DefinedSchemas
after defined schemas, call server Start complete

sadortun · 2021-11-08T14:53:36Z

@Moumouls

Your suggestions make sense,

Also just to clarify, the current [Parse release] init may or may not load CloudCode before or after the DB init sequence, since the DB init is made async.

Moving the CloudCode init before would make the whole init process deterministic, which is good.

Could it be possible that any user rely on the possibility that when CloudCode is loaded, DB was already initialized? I don't think so, but it might be something to consider if it could be a BC break if we fix this inconsistency.

sadortun · 2021-11-10T16:14:12Z

@mtrezza do you still plan in merging this (once updated)? I think I saw you wanted to wait in 2022?

LMK, I have some time today to finish it so it could be ready later today.

This PR should not introduce any BC break

mtrezza · 2021-11-11T00:07:22Z

If there is no breaking change, we can merge this also into beta, as it seems to contain a fix. Is there anything that needs to change in this PR to incorporate the feedback above? Could you bring this up-to-date with alpha?

sadortun · 2021-11-11T00:20:36Z

Is there anything that needs to change in this PR to incorporate the feedback above?

Yep, I'll do that in ~1-2 hours. @Moumouls prochaine vides really great feedback

Could you bring this up-to-date with alpha?

Will do.

Moumouls · 2021-11-11T08:15:51Z

Yep, I'll do that in ~1-2 hours. @Moumouls prochaine vides really great feedback

I try to do my best here 🙂

I'll take a look here

Moumouls · 2021-11-11T08:22:59Z

src/ParseServer.js

+    Promise.resolve()
+      .then(async () => await databaseController.performInitialization())
+      .then(async () => await hooksController.load())
      .then(async () => {
        if (schema) {
          await new DefinedSchemas(schema, this.config).execute();
        }
+      })
+      .then(async () => {
        if (serverStartComplete) {
-          serverStartComplete();
+          await serverStartComplete();
        }
      })
-      .catch(error => {
+      .catch(async (error) => {
        if (serverStartComplete) {
-          serverStartComplete(error);
+          await serverStartComplete(error);
        } else {
          console.error(error);
          process.exit(1);
        }
      });


Just to help to cross de finish line, here the full correct structure.

// Note: Tests will start to fail if any validation happens after this is called. databaseController.performInitialization() .then(() => hooksController.load()) .then(async () => { if (schema) { await new DefinedSchemas(schema, this.config).execute(); } }) .then(async () => { if (serverStartComplete) { await Promise.resolve(serverStartComplete()); } }) .catch(async (error) => { if (serverStartComplete) { await Promise.resolve(serverStartComplete(error)); } else { console.error(error); process.exit(1); } });

Then in a futur PR that refactor the start up (BK change) the structure will be

try { await databaseController.performInitialization() await hooksController.load() if (schema) { await new DefinedSchemas(schema, this.config).execute(); } if (serverStartComplete) { await Promise.resolve(serverStartComplete()); } } catch(e){ if (serverStartComplete) { await Promise.resolve(serverStartComplete(error)); } else { console.error(error); process.exit(1); } }

Mmm, not sure what's the difference.

I don't remember where, but we had a discussion about starting Promises chains with Promise.resolve() to be consistent with other promises chains in Server and JS SDK

await Promise.resolve(serverStartComplete());

I don't think we need to wrap it around Promise.resolve, await will work either way.

mtrezza · 2021-11-11T12:49:33Z

Just a reminder that breaking changes are phased-in. So if we know already that a breaking change will be needed in the future, this PR should already deprecate the current approach and provide the new approach optionally.

dblythy · 2022-12-23T08:54:31Z

Closing as this feature is now part of the alpha branch

sadortun force-pushed the pr_startupcomplete branch from 0d30231 to 4ca73f6 Compare September 3, 2021 16:14

mtrezza reviewed Sep 3, 2021

View reviewed changes

src/ParseServer.js Show resolved Hide resolved

sadortun force-pushed the pr_startupcomplete branch from a50c34b to 7750e52 Compare September 3, 2021 16:47

dblythy reviewed Sep 4, 2021

View reviewed changes

src/ParseServer.js Show resolved Hide resolved

sadortun force-pushed the pr_startupcomplete branch from 7750e52 to 015be02 Compare October 5, 2021 06:57

mtrezza changed the title ~~Ensure the startup is completed before calling startupCompleted~~ fix: ensure the startup is completed before calling startupCompleted Oct 9, 2021

dblythy self-requested a review October 9, 2021 00:37

dblythy suggested changes Oct 9, 2021

View reviewed changes

src/ParseServer.js Outdated Show resolved Hide resolved

src/ParseServer.js Outdated Show resolved Hide resolved

azlekov mentioned this pull request Oct 13, 2021

feat: add user-defined schema and migrations #7418

Merged

6 tasks

Moumouls mentioned this pull request Nov 5, 2021

refactor: allow ES import for cloud string if package type is module #7560

Merged

4 tasks

mtrezza mentioned this pull request Nov 5, 2021

Cloud Code import can cause race condition #7680

Closed

4 tasks

Moumouls suggested changes Nov 8, 2021

View reviewed changes

Ensure the startup is completed before calling startupCompleted

0e6ecba

sadortun force-pushed the pr_startupcomplete branch from 015be02 to 0e6ecba Compare November 11, 2021 01:19

Moumouls suggested changes Nov 11, 2021

View reviewed changes

jonas-db mentioned this pull request Dec 22, 2021

Parse Cloud Code Function won't allow Queries, or any object manipulation through cloud code. #7728

Closed

mtrezza force-pushed the alpha branch from 59215e6 to e6d7d8f Compare May 1, 2022 02:29

dblythy closed this Dec 23, 2022

fix: ensure the startup is completed before calling startupCompleted #7525

fix: ensure the startup is completed before calling startupCompleted #7525

Conversation

sadortun commented Aug 27, 2021 • edited Loading

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

mtrezza commented Aug 27, 2021 • edited Loading

dblythy commented Sep 3, 2021

sadortun commented Sep 3, 2021

sadortun commented Sep 3, 2021

mtrezza commented Sep 3, 2021

sadortun commented Sep 3, 2021

mtrezza commented Sep 3, 2021 • edited Loading

mtrezza commented Sep 3, 2021 • edited Loading

sadortun commented Sep 3, 2021

sadortun commented Sep 3, 2021 • edited Loading

mtrezza commented Sep 3, 2021 • edited Loading

sadortun commented Sep 3, 2021

mtrezza commented Sep 3, 2021

sadortun commented Sep 3, 2021

codecov bot commented Sep 3, 2021 • edited Loading

Codecov Report

mtrezza commented Sep 3, 2021

sadortun commented Sep 3, 2021

mtrezza commented Sep 9, 2021

sadortun commented Sep 9, 2021

mtrezza commented Sep 9, 2021 • edited Loading

sadortun commented Oct 5, 2021

mtrezza commented Oct 8, 2021 • edited Loading

sadortun commented Oct 8, 2021

parse-github-assistant bot commented Oct 9, 2021

Thanks for opening this pull request!

mtrezza commented Oct 9, 2021 • edited Loading

dblythy left a comment

Choose a reason for hiding this comment

mtrezza commented Oct 13, 2021

Moumouls commented Oct 14, 2021 • edited Loading

Moumouls commented Oct 14, 2021 • edited Loading

mtrezza commented Nov 6, 2021

mtrezza commented Nov 6, 2021 • edited Loading

Moumouls left a comment

Choose a reason for hiding this comment

sadortun commented Nov 8, 2021

sadortun commented Nov 10, 2021 • edited Loading

mtrezza commented Nov 11, 2021 • edited Loading

sadortun commented Nov 11, 2021

Moumouls commented Nov 11, 2021

Moumouls Nov 11, 2021

Choose a reason for hiding this comment

sadortun Nov 11, 2021

Choose a reason for hiding this comment

mtrezza commented Nov 11, 2021 • edited Loading

dblythy commented Dec 23, 2022

fix: ensure the startup is completed before calling `startupCompleted` #7525

fix: ensure the startup is completed before calling `startupCompleted` #7525

sadortun commented Aug 27, 2021 •

edited

Loading

mtrezza commented Aug 27, 2021 •

edited

Loading

mtrezza commented Sep 3, 2021 •

edited

Loading

mtrezza commented Sep 3, 2021 •

edited

Loading

sadortun commented Sep 3, 2021 •

edited

Loading

mtrezza commented Sep 3, 2021 •

edited

Loading

codecov bot commented Sep 3, 2021 •

edited

Loading

mtrezza commented Sep 9, 2021 •

edited

Loading

mtrezza commented Oct 8, 2021 •

edited

Loading

mtrezza commented Oct 9, 2021 •

edited

Loading

Moumouls commented Oct 14, 2021 •

edited

Loading

Moumouls commented Oct 14, 2021 •

edited

Loading

mtrezza commented Nov 6, 2021 •

edited

Loading

sadortun commented Nov 10, 2021 •

edited

Loading

mtrezza commented Nov 11, 2021 •

edited

Loading

mtrezza commented Nov 11, 2021 •

edited

Loading