perf: Apollo parallel server init under load #9929
Open
+46
−15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! ❌ Please fill out all fields with a placeholder |
📝 WalkthroughWalkthroughAdds a concurrency guard and promise-based initialization to ParseGraphQLServer._getServer(), and a test that verifies 100 parallel _getServer() calls resolve to the same server reference. Changes
Sequence Diagram(s)sequenceDiagram
participant CallerA
participant CallerB
participant ParseGraphQLServer
participant Mutex
participant ServerInit
Note over CallerA,CallerB: Parallel callers request server
CallerA->>ParseGraphQLServer: _getServer()
CallerB->>ParseGraphQLServer: _getServer()
ParseGraphQLServer->>Mutex: check/acquire _schemaRefMutex
Mutex-->>ParseGraphQLServer: lock/allow creation OR attach to existing promise
alt first creator
ParseGraphQLServer->>ServerInit: createServer() (returns promise)
ServerInit-->>ParseGraphQLServer: resolves with Server instance
ParseGraphQLServer-->>CallerA: resolves with Server
ParseGraphQLServer-->>CallerB: resolves with same Server (await same promise)
else attaching caller
ParseGraphQLServer-->>CallerB: returns existing init promise
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-05-09T09:59:06.289ZApplied to files:
📚 Learning: 2025-05-09T09:59:06.289ZApplied to files:
📚 Learning: 2025-05-04T20:41:05.147ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Member
Author
|
@Stovedal, thanks for your report, it could lead to a significant performance gain during startup under heavy load. |
Moumouls
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/ParseGraphQLServer.spec.js (1)
121-134: Good concurrency test; tighten assertions and reset.Looks solid and reflects the intended mutex behavior. Two nits:
- Prefer a single assertion using a Set to ensure all references are identical.
- If the implementation now stores the in-flight promise on
_server, resettingservermay not cold‑start the path. Consider clearing the correct slot (or adding a tiny helper) to avoid false positives.- it('should return same server reference when called 100 times in parallel', async () => { - parseGraphQLServer.server = undefined; + it('should return same server reference when called 100 times in parallel', async () => { + // Ensure cold start regardless of internal field name. + parseGraphQLServer.server = undefined; + if ('_server' in parseGraphQLServer) parseGraphQLServer._server = undefined; @@ - // All resolved servers should be the same reference - const firstServer = servers[0]; - servers.forEach((server, index) => { - expect(server).toBe(firstServer); - }); + // All resolved servers should be the same reference + const unique = new Set(servers); + expect(unique.size).toBe(1); });If feasible, also spy on the underlying server factory to assert it runs once (e.g.,
spyOn(parseGraphQLServer, '_createServer')or equivalent) to make the test stricter. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/ParseGraphQLServer.spec.js(1 hunks)src/GraphQL/ParseGraphQLServer.js(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/ParseGraphQLServer.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/ParseGraphQLServer.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/ParseGraphQLServer.spec.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/ParseGraphQLServer.spec.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/ParseGraphQLServer.spec.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
spec/ParseGraphQLServer.spec.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Benchmarks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9929 +/- ##
==========================================
- Coverage 93.07% 93.06% -0.02%
==========================================
Files 187 187
Lines 15224 15233 +9
Branches 177 177
==========================================
+ Hits 14170 14176 +6
- Misses 1042 1045 +3
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…uls/parse-server into moumouls/fix-apollo-parallel-start
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
Issue
Underload, apollo server can be initialized multiple times. Schema ref should be used as mutex to prevent parallel init, so all _getServer await the same promise ref
Closes: FILL_THIS_OUT
Approach
Using schema as a ref mutex
Tasks
Summary by CodeRabbit
Bug Fixes
Tests