fix(core): only start plugin workers once #22222

FrozenPandaz · 2024-03-07T23:02:28Z

Current Behavior

In the daemon, plugin workers will be loaded and shutdown concurrently by different processes. This causes requests to be unanswered and for the main process to hang.

Expected Behavior

The main Nx process does not hang and the requests are fulfilled. The daemon starts the plugin workers once and uses them until it shuts down.

Related Issue(s)

Fixes #

vercel · 2024-03-07T23:02:32Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated (UTC)
nx-dev	⬜️ Ignored (Inspect)	Visit Preview	Mar 8, 2024 2:27pm

nx-cloud · 2024-03-07T23:03:15Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4c2c2f3. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 6 targets

Sent with 💌 from NxCloud.

AgentEnder

This looks good in general. I agree with returning the callbacks to shut plugins down, and then the overall callback from loadNxPlugins would shutdown all of the plugins from that run. I think we need to be a bit more careful s.t. we arent calling shutdownPluginWorkers multiple times when multiple plugins are in use. Imagine we have 5 plugins. Currently, this looks like:

Load Nx Plugins ->

Load Nx Plugin * 5
- Each returned cleanup fn shutsdown all 5 workers
Returns a cleanup fn that calls all 5 cleanup fns
cleanup()
Calls returned cleanup fn once
- Calls all 5 sub-cleanup functions
  - Calls shutdownPluginWorkers which tries to kill all 5 workers

Its probably fine , but is a bit messy.

AgentEnder · 2024-03-08T03:11:22Z

packages/nx/src/project-graph/plugins/internal-api.ts

+export async function loadNxPluginAsync(
+  pluginConfiguration: PluginConfiguration,
+  paths: string[],
+  projects: Record<string, ProjectConfiguration>,
+  root: string
+): Promise<LoadedNxPlugin> {
+  const { plugin: moduleName, options } =
+    typeof pluginConfiguration === 'object'
+      ? pluginConfiguration
+      : { plugin: pluginConfiguration, options: undefined };
+
+  performance.mark(`Load Nx Plugin: ${moduleName} - start`);
+  let { pluginPath, name } = await getPluginPathAndName(
+    moduleName,
+    paths,
+    projects,
+    root
+  );
+  const plugin = normalizeNxPlugin(await importPluginModule(pluginPath));
+  plugin.name ??= name;
+  performance.mark(`Load Nx Plugin: ${moduleName} - end`);
+  performance.measure(
+    `Load Nx Plugin: ${moduleName}`,
+    `Load Nx Plugin: ${moduleName} - start`,
+    `Load Nx Plugin: ${moduleName} - end`
+  );
+  return { plugin, options };
+}


I'm really not a big fan of this making it over into the internal-api file. Ideally, The plugin worker should only have to import from worker-api and utils, to ensure that there is a clean separation between the stuff happening there and the rest of the code.

Additionally, if a plugin were to set its create nodes pattern based on the result of a config file (similar to how the package-json-workspaces plugin works) we could very easily hit a situation where swc-node and ts-node are both registered on main thread. Imagine if instead of package.json having the workspaces field it was something like workspace.ts that required a transpiler.

If the only reason we are doing this is to support listing createNodes and createDependencies support in nx list, I'd honestly rather drop that support and we could add some field to package.json if a plugin wants to list that they support it or similar. e.g. something like

{ "nx-plugin": { "generators": "generators.json" "executors": "executors.json" "graph": ... // this could be a boolean to indicate support, but maybe a path to import would work well here. Then we would be able to drop the /plugin at the end of a lot of our PCV3 plugins 🤔 . } }

AgentEnder · 2024-03-08T03:12:24Z

packages/nx/src/project-graph/plugins/plugin-pool.ts

+  return [
+    new Promise<RemotePlugin>((res, rej) => {
+      worker.on('message', createWorkerHandler(worker, res, rej));
+      worker.on('exit', createWorkerExitHandler(worker));
+    }),
+    () => {
+      shutdownPluginWorkers();
+    },
+  ] as const;


I feel like this should be an object so we have more well defined labels (e.g.)

return { plugin: RemotePlugin shutdown: () => Promise<void> }

Mm.. Idk how much of a difference it makes. I'm going to keep it a tuple for the interest of time.

AgentEnder · 2024-03-08T03:12:53Z

packages/nx/src/project-graph/plugins/plugin-pool.ts

+      worker.on('exit', createWorkerExitHandler(worker));
+    }),
+    () => {
+      shutdownPluginWorkers();


This callback should probably shut down the single plugin that was just loaded. (e.g. shutdownPluginWorker(worker))

AgentEnder · 2024-03-08T03:13:12Z

packages/nx/src/project-graph/plugins/plugin-pool.ts

 }

 let pluginWorkersShutdown = false;

-export async function shutdownPluginWorkers() {
+async function shutdownPluginWorkers() {


If we make the changes above, this becomes shutdownPluginWorker

AgentEnder · 2024-03-08T03:14:48Z

packages/nx/src/project-graph/plugins/internal-api.ts


-export async function loadNxPlugins(
+export async function loadNxPluginsRemotely(


Suggested change

export async function loadNxPluginsRemotely(

export async function loadNxPluginsViaWorker(

AgentEnder · 2024-03-08T03:15:22Z

packages/nx/src/project-graph/plugins/internal-api.ts

 }

-export async function loadNxPlugin(
+export function loadNxPluginRemotely(


Suggested change

export function loadNxPluginRemotely(

export function loadNxPluginViaWorker(

AgentEnder · 2024-03-08T03:17:29Z

packages/nx/src/utils/plugins/plugin-capabilities.ts

  try {
    return packageJson.generators ??
      packageJson.executors ??
      packageJson['nx-migrations'] ??
      packageJson['schematics'] ??
      packageJson['builders']
-      ? await loadRemoteNxPlugin(packageJson.name, workspaceRoot)
+      ? (await loadPlugin(packageJson.name, workspaceRoot)).plugin


See earlier lengthy comment. If this is only reason to expose loading a plugin on the main thread, lets just not. Its very little benefit to doing it and introduces all of the issues associated with loading modules during this stuff. Let's see about deferring to a package.json field like everything else.

It's not worth starting up a worker for this at the moment. At least for now, it's fine to require it just for metadata

I hear you, I'm not necessarily suggesting we start workers up, but if not then let's just drop support for list mentioning graph stuff for the time being and we can add that support back in later via a package json Metadata field or similar.

FrozenPandaz

Thanks for the review @AgentEnder.

FrozenPandaz · 2024-03-08T05:46:42Z

packages/nx/src/project-graph/plugins/plugin-pool.ts

+  return [
+    new Promise<RemotePlugin>((res, rej) => {
+      worker.on('message', createWorkerHandler(worker, res, rej));
+      worker.on('exit', createWorkerExitHandler(worker));
+    }),
+    () => {
+      shutdownPluginWorkers();
+    },
+  ] as const;


Mm.. Idk how much of a difference it makes. I'm going to keep it a tuple for the interest of time.

FrozenPandaz · 2024-03-08T05:48:34Z

packages/nx/src/utils/plugins/plugin-capabilities.ts

  try {
    return packageJson.generators ??
      packageJson.executors ??
      packageJson['nx-migrations'] ??
      packageJson['schematics'] ??
      packageJson['builders']
-      ? await loadRemoteNxPlugin(packageJson.name, workspaceRoot)
+      ? (await loadPlugin(packageJson.name, workspaceRoot)).plugin


It's not worth starting up a worker for this at the moment. At least for now, it's fine to require it just for metadata

AgentEnder · 2024-03-08T07:54:48Z

packages/nx/src/project-graph/plugins/plugin-pool.ts

-  }).then((val) => {
-    // Remove the promise from the pending set
-    pending.delete(tx);
-    // Return the original value
-    return val;
  });


Why do we not remove promises from pending when they complete now?

AgentEnder · 2024-03-08T07:57:18Z

packages/nx/src/utils/plugins/plugin-capabilities.ts

  try {
    return packageJson.generators ??
      packageJson.executors ??
      packageJson['nx-migrations'] ??
      packageJson['schematics'] ??
      packageJson['builders']
-      ? await loadRemoteNxPlugin(packageJson.name, workspaceRoot)
+      ? (await loadPlugin(packageJson.name, workspaceRoot)).plugin


I hear you, I'm not necessarily suggesting we start workers up, but if not then let's just drop support for list mentioning graph stuff for the time being and we can add that support back in later via a package json Metadata field or similar.

This reverts commit ec12e67.

github-actions · 2024-03-15T00:07:24Z

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

FrozenPandaz force-pushed the fix-pool2 branch 3 times, most recently from 349635f to aedac1b Compare March 8, 2024 01:43

FrozenPandaz marked this pull request as ready for review March 8, 2024 02:52

FrozenPandaz requested review from a team as code owners March 8, 2024 02:52

FrozenPandaz requested review from Cammisuli and JamesHenry March 8, 2024 02:52

FrozenPandaz enabled auto-merge (squash) March 8, 2024 02:55

AgentEnder requested changes Mar 8, 2024

View reviewed changes

FrozenPandaz force-pushed the fix-pool2 branch from 3015877 to 3f1153b Compare March 8, 2024 05:51

FrozenPandaz commented Mar 8, 2024

View reviewed changes

FrozenPandaz force-pushed the fix-pool2 branch from 3f1153b to becfd41 Compare March 8, 2024 06:10

AgentEnder reviewed Mar 8, 2024

View reviewed changes

fix(core): only start plugin workers once

6235e6a

FrozenPandaz force-pushed the fix-pool2 branch 2 times, most recently from a17ef8b to b1dabae Compare March 8, 2024 14:24

cleanup(core): refactor plugin shutdown

4c2c2f3

FrozenPandaz force-pushed the fix-pool2 branch from b1dabae to 4c2c2f3 Compare March 8, 2024 14:25

AgentEnder approved these changes Mar 8, 2024

View reviewed changes

FrozenPandaz merged commit ec12e67 into nrwl:master Mar 8, 2024
6 checks passed

FrozenPandaz deleted the fix-pool2 branch March 8, 2024 15:34

JamesHenry pushed a commit to JamesHenry/nx that referenced this pull request Mar 8, 2024

fix(core): only start plugin workers once (nrwl#22222)

8fc347b

JamesHenry pushed a commit to JamesHenry/nx that referenced this pull request Mar 8, 2024

fix(core): only start plugin workers once (nrwl#22222)

04c0da9

FrozenPandaz added a commit to FrozenPandaz/nx that referenced this pull request Mar 8, 2024

Revert "fix(core): only start plugin workers once (nrwl#22222)"

548fcd6

This reverts commit ec12e67.

FrozenPandaz added a commit to FrozenPandaz/nx that referenced this pull request Mar 8, 2024

Revert "fix(core): only start plugin workers once (nrwl#22222)"

e6dbe80

This reverts commit ec12e67.

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): only start plugin workers once #22222

fix(core): only start plugin workers once #22222

FrozenPandaz commented Mar 7, 2024 •

edited

vercel bot commented Mar 7, 2024 •

edited

nx-cloud bot commented Mar 7, 2024 •

edited

AgentEnder left a comment

AgentEnder Mar 8, 2024

AgentEnder Mar 8, 2024

FrozenPandaz Mar 8, 2024

AgentEnder Mar 8, 2024

AgentEnder Mar 8, 2024

AgentEnder Mar 8, 2024

AgentEnder Mar 8, 2024

AgentEnder Mar 8, 2024

FrozenPandaz Mar 8, 2024

AgentEnder Mar 8, 2024

FrozenPandaz left a comment

FrozenPandaz Mar 8, 2024

FrozenPandaz Mar 8, 2024

AgentEnder Mar 8, 2024

AgentEnder Mar 8, 2024

github-actions bot commented Mar 15, 2024


		export async function loadNxPlugins(
		export async function loadNxPluginsRemotely(

	export async function loadNxPluginsRemotely(
	export async function loadNxPluginsViaWorker(

	export function loadNxPluginRemotely(
	export function loadNxPluginViaWorker(

fix(core): only start plugin workers once #22222

fix(core): only start plugin workers once #22222

Conversation

FrozenPandaz commented Mar 7, 2024 • edited

Current Behavior

Expected Behavior

Related Issue(s)

vercel bot commented Mar 7, 2024 • edited

nx-cloud bot commented Mar 7, 2024 • edited

☁️ Nx Cloud Report

AgentEnder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrozenPandaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 15, 2024

FrozenPandaz commented Mar 7, 2024 •

edited

vercel bot commented Mar 7, 2024 •

edited

nx-cloud bot commented Mar 7, 2024 •

edited