fix(node): Cooperative Scheduling between startBuild and subProcess generators #7110

dpchamps · 2021-09-23T16:05:27Z

Current Behavior

This pr patches the bug outlined here: #7031

The node:execute builder immediately exits after build when --watch=false.

Expected Behavior

The node:execute builder wait for the subprocess to exit, or SIGTERM to be received, before exiting. This is how things worked previously.

Related Issue(s)

Fixes #7031

Notes

The solution here is to essentially "cooperatively schedule" (using the term loosely) events emitted from the startBuild generator, and the events emitted from the child process.

We must allow build events to propagate (for example, when watch mode is configured), while also ensuring that the executor waits for the current process to run to completion.

I've implemented some things as classes here, because my sense was this was more in the style of the repo. Happy to capture them differently, if the team would like. Have dropped some github comments along the way, happy to discuss the impl furthur!

vercel · 2021-09-23T16:05:32Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nrwl/nx-dev/HPDNg9xaTVrx9CvSXgYsrYe5C7ei
✅ Preview: Canceled

[Deployment for 26a1a0e canceled]

dpchamps · 2021-09-23T16:07:41Z

packages/node/src/executors/execute/execute.impl.spec.ts

-      return {
-        on: (eventName, cb) => {
+      mockSubProcess = {
+        on: jest.fn().mockImplementation((eventName, cb) => {


Represents a breaking change: the type of event yielded by executeExecutor is now: https://github.com/nrwl/nx/pull/7110/files#diff-8b1daad050dbc745646b89f91cd20c497a23f51caf7eeaddb96a517a80b8f01fR28

dpchamps · 2021-09-23T16:08:05Z

packages/node/src/executors/execute/execute.impl.spec.ts

@@ -215,6 +220,10 @@ describe('NodeExecuteBuilder', () => {
  });

  it('should log errors from killing the process', async () => {
+    (devkit.runExecutor as any).mockImplementation(function* () {


The old test cases for this spec and the one below were non-deterministic:

The state of subProcess was managed as a file-level global previously. These tests would only pass when at least one previous test had been run to populate the subProcess var.

With that subProcess state encapsulated, there was no way to make this test pass without altering it.

packages/node/src/executors/execute/execute.impl.ts

dpchamps · 2021-09-23T16:09:41Z

packages/node/src/executors/execute/execute.impl.ts

+  ]);
+
+  while (iteratorMap.size > 0) {
+    const result = await Promise.race(iteratorMap.values());


This is the crux of the sln, greedily take the next ready value.

Consume each individual iterator lazily: Only tick the last iterator to have won the race. On Build event, reset the state of the subProcess iterator

dpchamps · 2021-09-23T16:10:21Z

packages/node/src/utils/types.ts

@@ -97,3 +98,15 @@ export interface NormalizedBuildNodeBuilderOptions
  extends BuildNodeBuilderOptions {
  webpackConfig: string[];
 }
+
+export interface NodeExecuteBuilderOptions {


I moved this type into utils, because SubProcessRunner uses it as well.

dpchamps · 2021-09-23T16:11:07Z

packages/node/src/utils/subProcessRunner.ts

+      error: unknown;
+    };
+
+export class SubProcessRunner {


Happy to write tests for this, awaiting feedback for where this should be in the project structure. It seems like it belongs colocated to executor.impl.ts, but I didn't see other examples of doing stuff like that.

The executor.impl tests cover the integration of this in full, but it felt big enough to split into a separate file. Let me know what you think!

llwt · 2021-09-23T17:12:15Z

packages/node/src/executors/execute/execute.impl.spec.ts

+        const result = await Promise.race([
+          executorIterator.next(),
+          new Promise(() => {
+            mockSubProcessExit(1);
+            events.push(firstEvent);
+          }),
+        ]);


Suggested change

const result = await Promise.race([

executorIterator.next(),

new Promise(() => {

mockSubProcessExit(1);

events.push(firstEvent);

}),

]);

const result = await Promise.race([

executorIterator.next(),

new Promise(() => {

mockSubProcessExit(1);

events.push(firstEvent);

// NOTE: this promise intentionally never resolves so that...

}),

]);

This threw me off for a sec :) Might help to leave some breadcrumbs

packages/node/src/executors/execute/execute.impl.spec.ts

packages/node/src/executors/execute/execute.impl.ts

nx-cloud · 2021-09-23T19:26:49Z

Nx Cloud Report

CI is running for commit 26a1a0e.

📂 Click to track the progress, see the status, the terminal output, and the build insights.

Sent with 💌 from NxCloud.

Use a kind of mergy-cooperative scheduling pattern, and queify subprocess handler

…generators

FrozenPandaz

This seems way too complicated to fix the issue outlined in #7031

I believe the fix should be that executeExecutor should not yield until the subprocess dies?

The executeExecutor keeps iterating over the build events
When it gets a build event, it starts the process
Currently, it yields.. I don't think it should do this yet?
It should wait until the subprocess exits to yield..

In general, we should be yielding results of executing the process here 👀

cc @AgentEnder could you take a look at this please?

dpchamps · 2021-09-25T00:27:17Z

@FrozenPandaz I agree it's a non trivial fix:

From the issue:

One further thing that's tripping me up a bit here is we'd like to also add support for using process.send to send the server URL up to the builder and trigger an iteration/emission (specifically for getting the started port in our e2e tests when we start a server on port: 0). I don't see an obvious way to do it without merging async iterables but maybe y'all see something obvious?

After speaking with @llwt, we need ideally two things:

To yield sub-process events as they occur (@llwt is specifically asking for message).
But also allow for build events to come through as needed.

Consider your points 3 and 4:

3. Currently, it yields.. I don't think it should do this yet?
4. It should wait until the subprocess exits to yield..

How do we then both wait for the subprocess to complete (and also yield events from that subprocess) but also allow for new build events to propagate?

For example, when I'm in watch mode, I don't want the subprocess to hold up a new build event. But I also want to make sure that the process exits before the executor completes. So we run into a situation where we need to simultaneously wait for the startBuild iterator to complete and also the most recent subprocess.

More context, if needed

This is all happening within a `for await ... of` loop. Where we need to run one iterator to completion, but wait for at least the last side-effect to complete before returning. It's further complicated by the requirement of yielding events from that side-effect.

So if subprocess events should be yielded here, then (as far as I can see) there's no way to do it without merging the two iterators to allow them to both "tick". SubProcess being push-based doesn't help the situation. I rolled the implementations myself to take an event emitter -> async iterator, but I'm sure there are third party libs to do that as well.

Make sense? If there's a more obvious solution I'm happy to talk it out :)

llwt · 2021-10-19T15:22:01Z

Bump @vsavkin / @AgentEnder,

Anything we can do to move this along? Currently serving an app outside of watch mode is not possible and we're in a broken state since updating to the latest version of nx.

Happy to modify the PR, but we need a bit of guidance as to what ya'll would like to see. We don't really see a simpler way to do this with the change to async iterators, however, maybe your eyes see something we don't?

AgentEnder · 2021-10-19T15:26:48Z

I'll take a look at this over the week, sorry for the delay.

llwt · 2021-10-20T15:15:26Z

Thanks!

cdaringe · 2021-11-12T22:38:25Z

Hey all,

I just want to chime in with a respectful, kind plea for attention here. We've been blocked on this for a while now, and are eager to pay back the techdebt accrued while blocked by a fix. @dpchamps explanation is above is fine and good, as is @llwt 's in #7710. i want to emphasize how fundamental of a problem this is, perhaps to elevate the priority here.

here's what we cant do right now:

// my-executor.ts
runExecutor(nodeExecutorInput1)
runExecutor(nodeExecutorInput2)

can't do it.

this is because execute.impl.js does this:

function runProcess(event, options) {
    if (subProcess || !event.success) {
        return rxjs_1.EMPTY;
    }
    return new rxjs_1.Observable((subscriber) => {
        subProcess = child_process_1.fork(event.outfile, options.args, { // 👀 👀 👀
            execArgv: getExecArgv(options),
        });

both executions race to squash the subProcess, and it creates problemos. Global, mutable singleton, getting crushed by competition.

We are willing to pair and discuss this further! :) I thought i'd try my hand in articulating the problem from a different perspective. Thanks

AgentEnder · 2021-12-14T20:25:45Z

@dpchamps Ok, I know I've said I'd look over this before but I'm going to spend some real time w/ this problem this week.

At minimum this PR would need to be rebased, but lets chat some on the issue first to make sure this is still the solution that we should pursue.

AgentEnder · 2021-12-14T20:33:13Z

@cdaringe it seems like this issue might be a bit different to the above discussion, albeit it would disappear if you were able to wait for the first executor to finish... From the code sample it sounds more like you want to be able to run two node execute runs in parallel from a custom executor. It may be a good idea to open a new issue for that.

dpchamps · 2021-12-14T23:13:13Z

@AgentEnder I met with @vsavkin a while back and he agreed to take these changes into a new more general executor. The "complicatedness" comes mostly from poor naming: its not "cooperative scheduling" so much as it is a need for some type merge+prioritization for two generators.

After the discussion with Victor, we arrived at the general conclusion that these changes did not appear to be too complicated for the problem at hand.

The issue that @cdaringe is calling out is fixed in this PR due to some non-determinism caused by having module-level mutable state.

jaysoo · 2022-01-27T13:47:14Z

Hi @dpchamps Thanks so much for your contribution. I'm going to close this PR since we are moving focus into the @nrwl/js:node executor, which is meant to run a build then execute (same as @nrwl/node:execute).

I will double check if the issue outlined in #7031 is still present.

dpchamps · 2022-01-27T20:22:10Z

@jaysoo I'm curious if you and team are planning on addressing @llwt's request in #7031:

One further thing ... is we'd like to also add support for using process.send to send the server URL up to the builder and trigger an iteration/emission (specifically for getting the started port in our e2e tests when we start a server on port: 0)

This PR presented an idea for how to make that possible.

We very much need this functionality. It's arguably more important to us than the initial bug presented, and is what @vsavkin and myself spent most of our time discussing in the call.

github-actions · 2023-03-19T10:05:59Z

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.

vercel bot temporarily deployed to Preview September 23, 2021 16:05 Inactive

dpchamps commented Sep 23, 2021

View reviewed changes

packages/node/src/executors/execute/execute.impl.ts Outdated Show resolved Hide resolved

dpchamps commented Sep 23, 2021

View reviewed changes

dpchamps changed the title ~~Execute iterator exploration~~ fix(node): Cooperative Scheduling between startBuild and subProcess generators Sep 23, 2021

dpchamps marked this pull request as draft September 23, 2021 16:12

llwt reviewed Sep 23, 2021

View reviewed changes

vercel bot temporarily deployed to Preview September 23, 2021 19:26 Inactive

dpchamps marked this pull request as ready for review September 23, 2021 19:42

Steven Nance and others added 13 commits September 23, 2021 15:58

WIP

cc568e6

Sketch:

3540006

Use a kind of mergy-cooperative scheduling pattern, and queify subprocess handler

Sketch 2: more classy... idk if this fits the codebase more

1da6465

unchanged test code

c16c7ec

testing im place

50e30b0

feat(node): Cooperative Scheduling between startBuild and subProcess …

dbf1dab

…generators

chore(misc): docs update

ae46bb2

chore(misc): prettier

ad53bf1

chore(misc): prettier

393ef3f

chore(node): remove scraps

72cefb2

refactor(node): make scheduled builder more clear

213f9b9

refactor(node): make typings zen. fix tests

764a0d2

chore(node): make tests more explicit

26a1a0e

dpchamps force-pushed the execute-iterator-exploration branch from 3e6afa8 to 26a1a0e Compare September 23, 2021 23:00

vercel bot temporarily deployed to Preview September 23, 2021 23:00 Inactive

FrozenPandaz assigned AgentEnder Sep 24, 2021

FrozenPandaz requested changes Sep 24, 2021

View reviewed changes

AgentEnder assigned nartc Dec 14, 2021

vsavkin force-pushed the master branch 2 times, most recently from 3a99403 to 334c230 Compare January 6, 2022 18:35

vsavkin assigned jaysoo and unassigned AgentEnder and nartc Jan 20, 2022

jaysoo closed this Jan 27, 2022

cdaringe mentioned this pull request Mar 12, 2022

runExecutor fails running js:serve concurrently #9305

Closed

dpchamps mentioned this pull request May 17, 2022

The node:execute builder immediately exits after build when --watch=false. #7031

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(node): Cooperative Scheduling between startBuild and subProcess generators #7110

fix(node): Cooperative Scheduling between startBuild and subProcess generators #7110

dpchamps commented Sep 23, 2021

vercel bot commented Sep 23, 2021 •

edited

dpchamps Sep 23, 2021 •

edited

dpchamps Sep 23, 2021

dpchamps Sep 23, 2021

dpchamps Sep 23, 2021

dpchamps Sep 23, 2021

llwt Sep 23, 2021

nx-cloud bot commented Sep 23, 2021 •

edited

FrozenPandaz left a comment •

edited

dpchamps commented Sep 25, 2021 •

edited

llwt commented Oct 19, 2021

AgentEnder commented Oct 19, 2021

llwt commented Oct 20, 2021

cdaringe commented Nov 12, 2021

AgentEnder commented Dec 14, 2021

AgentEnder commented Dec 14, 2021

dpchamps commented Dec 14, 2021 •

edited

jaysoo commented Jan 27, 2022

dpchamps commented Jan 27, 2022 •

edited

github-actions bot commented Mar 19, 2023

fix(node): Cooperative Scheduling between startBuild and subProcess generators #7110

fix(node): Cooperative Scheduling between startBuild and subProcess generators #7110

Conversation

dpchamps commented Sep 23, 2021

Current Behavior

Expected Behavior

Related Issue(s)

Notes

vercel bot commented Sep 23, 2021 • edited

dpchamps Sep 23, 2021 • edited

Choose a reason for hiding this comment

dpchamps Sep 23, 2021

Choose a reason for hiding this comment

dpchamps Sep 23, 2021

Choose a reason for hiding this comment

dpchamps Sep 23, 2021

Choose a reason for hiding this comment

dpchamps Sep 23, 2021

Choose a reason for hiding this comment

llwt Sep 23, 2021

Choose a reason for hiding this comment

nx-cloud bot commented Sep 23, 2021 • edited

Nx Cloud Report

FrozenPandaz left a comment • edited

Choose a reason for hiding this comment

dpchamps commented Sep 25, 2021 • edited

llwt commented Oct 19, 2021

AgentEnder commented Oct 19, 2021

llwt commented Oct 20, 2021

cdaringe commented Nov 12, 2021

AgentEnder commented Dec 14, 2021

AgentEnder commented Dec 14, 2021

dpchamps commented Dec 14, 2021 • edited

jaysoo commented Jan 27, 2022

dpchamps commented Jan 27, 2022 • edited

github-actions bot commented Mar 19, 2023

vercel bot commented Sep 23, 2021 •

edited

dpchamps Sep 23, 2021 •

edited

nx-cloud bot commented Sep 23, 2021 •

edited

FrozenPandaz left a comment •

edited

dpchamps commented Sep 25, 2021 •

edited

dpchamps commented Dec 14, 2021 •

edited

dpchamps commented Jan 27, 2022 •

edited