-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(core): lock graph creation when running in another process #29408
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
View your CI Pipeline Execution ↗ for commit 78836c3.
☁️ Nx Cloud last updated this comment at |
d1f6a60
to
8bb1c07
Compare
@@ -290,6 +316,7 @@ export async function createProjectGraphAndSourceMapsAsync( | |||
'create-project-graph-async:start', | |||
'create-project-graph-async:end' | |||
); | |||
lock.unlock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in a finally
at least
packages/nx/src/utils/file-lock.ts
Outdated
} | ||
|
||
wait(timeout?: number) { | ||
return new Promise<void>((res, rej) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use polling
8bb1c07
to
d14b40b
Compare
d14b40b
to
41b174c
Compare
0204e02
to
beff91a
Compare
beff91a
to
b44f168
Compare
16db699
to
7c67700
Compare
#[napi(constructor)] | ||
pub fn new(lock_file_path: String) -> anyhow::Result<Self> { | ||
// Creates the directory where the lock file will be stored | ||
fs::create_dir_all(Path::new(&lock_file_path).parent().unwrap())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't unwrap this. Use ?
to return the Err
import { fork } from 'child_process'; | ||
import { join } from 'path'; | ||
|
||
describe('withLock', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename
if (graph.errors.length > 0) { | ||
throw new ProjectGraphError(graph.errors, graph, sourceMaps); | ||
} | ||
if (Date.now() - graph.computedAt < 10_000) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get the current time sooner before Nx even starts waiting. The graph will be computing... and should have been written after it has started waiting... This guarantees that it is not old.
Also handle when there is no graph after waiting.
7c67700
to
78836c3
Compare
Current Behavior
Running Nx in multiple processes at the same time with the daemon disabled can cripple a system due to excess memory usage when creating the graph. This is due to plugin workers being started per-parent process when there is no daemon. This change enables a file lock to prevent the simultaneous processing, and read from the cache when the first run completes.
Currently, running
nx show projects
30 times in parallel looks something like this:30 processes exited within 37535ms
Expected Behavior
30 processes exited within 6435ms
Test Script