Cloud Code import can cause race condition

Moumouls commented 3 years ago

New Issue Checklist

[x] I am not disclosing a vulnerability.
[x] I am not just asking a question.
[x] I have searched through existing issues.
[x] I can reproduce the issue with the latest version of Parse Server.

Issue Description

Module import is async, importing as module can currently cause a race issue. A developer do not have any guarantee that the code will be loaded before he try to run some cloud code functions. We have some recurrent CI fails due to https://github.com/parse-community/parse-server/pull/7560.

See https://github.com/parse-community/parse-server/pull/7560#issuecomment-962053007

Steps to reproduce

Actual Outcome

Random CI fails

Expected Outcome

No flacky

Environment

beta

Server

Parse Server version: 5.0.0-beta.1
Operating system: all
Local or remote host (AWS, Azure, Google Cloud, Heroku, Digital Ocean, etc): action

Database

System (MongoDB or Postgres): all
Database version: all
Local or remote host (MongoDB Atlas, mLab, AWS, Azure, Google Cloud, etc): all

Client

SDK (iOS, Android, JavaScript, PHP, Unity, etc): beta
SDK version: beta

Logs

parse-github-assistant[bot] commented 3 years ago

Thanks for opening this issue!

🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

Moumouls commented 3 years ago

Here as discussed with @mtrezza i'll recommend a simple revert.

mtrezza commented 3 years ago

Thanks @Moumouls. Just in case we want to remove the feature, we don't do pure git reverts, we require a new PR that removes the code. I suggest let's see what @dblythy suggests. I renamed the title because the issue it not purely a CI issue but has potential impact on cloud code loading as I understand it.

Moumouls commented 3 years ago

Yes this could have a real impact in prod environment and could lead to a cloud code crash.

mtrezza commented 3 years ago

The alternative to removing the feature would probably be to get @sadortun's https://github.com/parse-community/parse-server/pull/7525 ready to fix the async initialization of Parse Server. But maybe we'd discover more things to change and would be getting from one thing to the other. So removing the feature seems easiest for now, then re-add it after #7525 has been merged.

dblythy commented 3 years ago

I'm happy for the feature to be reverted (if this is the most suitable approach), however, what's the recommended way for someone who is using Parse Server as a ES6 module to use cloud code? Perhaps we could throw an error "ES6 Modules cannot use cloud code strings, use () => import('') instead.

Moumouls commented 3 years ago

Currently the clean ways is to first import the cloud code file function in a index.js then provide the cloud code function to Parse Server.

ex:

// job.js
export const jobs = () => {
  Parse.Could.Job(xxxxxxxx)
}

// beforeSaves.js
export const beforeSaves = () => {
  Parse.Could.beforeSave(xxxxxxxx)
}

// cloud.js
import { jobs} from './jobs'
import { beforeSaves } from './beforeSaves'

export const cloud = () => {
  // Lazy load all Parse.Cloud
  jobs()
  beforeSaves()
}

// index.js
import { cloud } from './cloud'

// Parse will execute sync the cloud function
// it's a lazy load strategy
ParseServer.start({ cloud })

This is my suggestion @dblythy until we have a clean refactor of ParseServer start 🙂

sadortun commented 3 years ago

I'll have a look at #7525 today. Lmk what's missing to be merged.

mtrezza commented 3 years ago

@sadortun that's great! Mind that we cannot merge any breaking changes in the alpha or beta branch anymore, since we already had the beta release. Any breaking change needs to be phased-in, giving an option to use the current functionality, or the new (breaking) functionality.

However, we need to merge a fix for this issue (#7680) in alpha and beta. So if #7525 is breaking and cannot be phased-in, we would need a solution that does not depend on #7525 to close this issue (#7680), which means probably just reverting.

sadortun commented 3 years ago

@mtrezza FYI, My PR #7525 have no breaking change.

dblythy commented 3 years ago

@sadortun in your PR, you removed the ES6 import() which is a breaking change, as those who use ES6 modules won't be able to specify a cloud code string

sadortun commented 3 years ago

@sadortun in your PR, you removed the ES6 import() which is a breaking change, as those who use ES6 modules won't be able to specify a cloud code string

Noted. That was not intended.

Moumouls commented 3 years ago

@mtrezza @dblythy here i'm sad to announce that ParseServer is currently not ready to support an async cloud code loading correctly without being sure not to break anything on certain productions.

We need an additional PR with a clean refactoring of ParseServer startup. But changing parse server startup from sync to async will lead to a breaking change.

Since we are at beta stage, @mtrezza I think it is wiser to remove the code and open a new issue tracking the refactor needs and how this breaking could be handled without creating any surprise on some production applications.

Here i'll suggest to remove the import code that allow cloud code file as ES Module. 🙁

mtrezza commented 3 years ago

@dblythy If you agree, would you want to open a PR to temporarily remove the feature in the alpha branch? We can re-add this in 2022 when we have more time to prepare https://github.com/parse-community/parse-server/pull/7525 without rush, which I think would be better as it seems to be somewhat tricky.

mtrezza commented 3 years ago

Preparing to revert with https://github.com/parse-community/parse-server/pull/7689 in alpha, https://github.com/parse-community/parse-server/pull/7691 in beta.

mtrezza commented 3 years ago

Closing as #7560 has been reverted in alpha and beta.

Thanks @dblythy for adding the feature, and I hope we can re-add it after #7525 has been merged.

parse-community / parse-server