Skip to content

Commit

Permalink
fix(core): isolated plugins should provide cleanup function (#26657)
Browse files Browse the repository at this point in the history
<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

<!-- If this is a particularly complex change or feature addition, you
can request a dedicated Nx release for this pull request branch. Mention
someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they
will confirm if the PR warrants its own release for testing purposes,
and generate it for you if appropriate. -->

## Current Behavior
- Isolated plugins return no-op cleanup function.

## Expected Behavior
- Isolated plugins cleanup fn shuts them down.

## Related Issue(s)
In #26625 we are starting to reload plugins when their configurations
change. This makes sense, but means we should actually shutdown + reload
workers.

Fixes #

(cherry picked from commit 3b2c42a)
  • Loading branch information
AgentEnder authored and FrozenPandaz committed Jun 26, 2024
1 parent 3e59e98 commit 38da368
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 15 deletions.
22 changes: 16 additions & 6 deletions packages/nx/src/project-graph/plugins/isolation/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,30 @@ import { loadRemoteNxPlugin } from './plugin-pool';
/**
* Used to ensure 1 plugin : 1 worker
*/
const remotePluginCache = new Map<string, Promise<LoadedNxPlugin>>();
const remotePluginCache = new Map<
string,
readonly [Promise<LoadedNxPlugin>, () => void]
>();

export function loadNxPluginInIsolation(
plugin: PluginConfiguration,
root = workspaceRoot
): [Promise<LoadedNxPlugin>, () => void] {
): readonly [Promise<LoadedNxPlugin>, () => void] {
const cacheKey = JSON.stringify(plugin);

if (remotePluginCache.has(cacheKey)) {
return [remotePluginCache.get(cacheKey), () => {}];
return remotePluginCache.get(cacheKey);
}

const loadingPlugin = loadRemoteNxPlugin(plugin, root);
remotePluginCache.set(cacheKey, loadingPlugin);
const [loadingPlugin, cleanup] = loadRemoteNxPlugin(plugin, root);
// We clean up plugin workers when Nx process completes.
return [loadingPlugin, () => {}];
const val = [
loadingPlugin,
() => {
cleanup();
remotePluginCache.delete(cacheKey);
},
] as const;
remotePluginCache.set(cacheKey, val);
return val;
}
22 changes: 14 additions & 8 deletions packages/nx/src/project-graph/plugins/isolation/plugin-pool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ interface PendingPromise {
export function loadRemoteNxPlugin(
plugin: PluginConfiguration,
root: string
): Promise<LoadedNxPlugin> {
): [Promise<LoadedNxPlugin>, () => void] {
// this should only really be true when running unit tests within
// the Nx repo. We still need to start the worker in this case,
// but its typescript.
Expand Down Expand Up @@ -66,13 +66,19 @@ export function loadRemoteNxPlugin(

cleanupFunctions.add(cleanupFunction);

return new Promise<LoadedNxPlugin>((res, rej) => {
worker.on(
'message',
createWorkerHandler(worker, pendingPromises, res, rej)
);
worker.on('exit', exitHandler);
});
return [
new Promise<LoadedNxPlugin>((res, rej) => {
worker.on(
'message',
createWorkerHandler(worker, pendingPromises, res, rej)
);
worker.on('exit', exitHandler);
}),
() => {
cleanupFunction();
cleanupFunctions.delete(cleanupFunction);
},
];
}

function shutdownPluginWorker(worker: ChildProcess) {
Expand Down
8 changes: 7 additions & 1 deletion packages/nx/src/project-graph/project-graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,13 @@ export async function buildProjectGraphAndSourceMapsWithoutDaemon() {
throw e;
}
} finally {
cleanup();
// When plugins are isolated we don't clean them up during
// a single run of the CLI. They are cleaned up when the CLI
// process exits. Cleaning them here could cause issues if pending
// promises are not resolved.
if (process.env.NX_ISOLATE_PLUGINS !== 'true') {
cleanup();
}
}

const { projectGraph, projectFileMapCache } = projectGraphResult;
Expand Down

0 comments on commit 38da368

Please sign in to comment.