Sitelet https://github.com/angular/angular/pull/36637
Skip to content

ngcc: new cluster worker entry-point - #36637

Closed
petebacondarwin wants to merge 12 commits into
angular:masterfrom
petebacondarwin:ngcc-worker-entry-point
Closed

ngcc: new cluster worker entry-point#36637
petebacondarwin wants to merge 12 commits into
angular:masterfrom
petebacondarwin:ngcc-worker-entry-point

Conversation

@petebacondarwin

@petebacondarwin petebacondarwin commented Apr 15, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

This refactoring simplifies the clustering by providing a new entry-point file for cluster workers.

This reduces the complexity of the code (not having to check for cluster.isMaster so much) and also removes the need to do parse and execute as much code in the worker processes.

@petebacondarwin petebacondarwin added state: WIP refactoring Issue that involves refactoring or code-cleanup comp: ngcc labels Apr 15, 2020
@ngbot ngbot Bot added this to the needsTriage milestone Apr 15, 2020
@petebacondarwin
petebacondarwin force-pushed the ngcc-worker-entry-point branch 4 times, most recently from 52c8f72 to 441dae7 Compare April 15, 2020 20:09
@petebacondarwin petebacondarwin added the target: patch This PR is targeted for the next patch release label Apr 15, 2020
@petebacondarwin
petebacondarwin force-pushed the ngcc-worker-entry-point branch 3 times, most recently from 78c8abb to e22f60b Compare April 16, 2020 10:00
@petebacondarwin
petebacondarwin marked this pull request as ready for review April 16, 2020 11:59
@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Apr 16, 2020
@pullapprove
pullapprove Bot requested a review from gkalpak April 16, 2020 11:59
gkalpak
gkalpak approved these changes Apr 16, 2020
View reviewed changes

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! 🎊 👌 💯

Comment thread packages/compiler-cli/ngcc/src/execution/cluster/master.ts Outdated
Comment thread packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts Outdated
Comment thread packages/compiler-cli/ngcc/src/main.ts Outdated
Comment thread packages/compiler-cli/ngcc/test/execution/cluster/package_json_updater_spec.ts Outdated
Comment thread packages/compiler-cli/ngcc/src/execution/cluster/worker.ts Outdated
Comment thread packages/compiler-cli/ngcc/test/execution/cluster/worker_spec.ts Outdated
Comment thread packages/compiler-cli/ngcc/src/execution/cluster/worker.ts Outdated
Comment thread packages/compiler-cli/ngcc/src/execution/cluster/worker.ts Outdated
@petebacondarwin petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 16, 2020
@petebacondarwin
fix(ngcc): display unlocker process output in sync mode
2042849 
The change in e041ac6
to support sending unlocker process output to the main ngcc
console output prevents messages require that the main process
relinquishes the event-loop to allow the `stdout.on()` handler to
run.  This results in none of the messages being written when ngcc
is run in `--no-async` mode, and some messages failing to be
written if the main process is killed (e.g. ctrl-C).

It appears that the problem with Windows and detached processes
is known - see nodejs/node#3596 (comment).
But in the meantime, this commit is a workaround, where non-Windows
`inherit` the main process `stdout` while on Windows it reverts
to the async handler approach, which is better than nothing.
@petebacondarwin
fixup! fix(ngcc): display unlocker process output in sync mode
c983e02
@petebacondarwin
refactor(ngcc): move command line option parsing to its own file
b0bb1ea
@petebacondarwin
refactor(ngcc): move analyze and compile functions into their own files
00ff990
@petebacondarwin
refactor(ngcc): move pathMapping processing to utils
af73b5c
@petebacondarwin
refactor(ngcc): create new entry-point for cluster workers
d6f9631
@petebacondarwin
fixup! refactor(ngcc): create new entry-point for cluster workers
3b2f6f0
@petebacondarwin
fixup! refactor(ngcc): create new entry-point for cluster workers
cf26700
@petebacondarwin
refactor(ngcc): simplify cluster PackageJsonUpdater
83005f9
@petebacondarwin
petebacondarwin force-pushed the ngcc-worker-entry-point branch 4 times, most recently from c3b5dfb to 9932209 Compare April 16, 2020 16:19
@petebacondarwin
refactor(ngcc): moved shared setup into a single function
ee34b76 
The `main.ts` and `worker.ts` had duplicate logic, which has now been
moved to a single function called `getSharedSetup()`.
@petebacondarwin
petebacondarwin force-pushed the ngcc-worker-entry-point branch from 9932209 to ee34b76 Compare April 16, 2020 16:28
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 16, 2020
@matsko matsko closed this in 2ed7146 Apr 16, 2020
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
fix(ngcc): display unlocker process output in sync mode (#36637)
cabf997 
The change in e041ac6
to support sending unlocker process output to the main ngcc
console output prevents messages require that the main process
relinquishes the event-loop to allow the `stdout.on()` handler to
run.  This results in none of the messages being written when ngcc
is run in `--no-async` mode, and some messages failing to be
written if the main process is killed (e.g. ctrl-C).

It appears that the problem with Windows and detached processes
is known - see nodejs/node#3596 (comment).
But in the meantime, this commit is a workaround, where non-Windows
`inherit` the main process `stdout` while on Windows it reverts
to the async handler approach, which is better than nothing.

PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
refactor(ngcc): move command line option parsing to its own file (#36637
3c14e96 
)

PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
refactor(ngcc): move analyze and compile functions into their own fil…
33df4b7 
…es (#36637)

PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
refactor(ngcc): move pathMapping processing to utils (#36637)
7e5e60b 
PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
refactor(ngcc): create new entry-point for cluster workers (#36637)
443f5ee 
PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
refactor(ngcc): simplify cluster PackageJsonUpdater (#36637)
bb944ee 
PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
refactor(ngcc): moved shared setup into a single function (#36637)
c332d4d 
The `main.ts` and `worker.ts` had duplicate logic, which has now been
moved to a single function called `getSharedSetup()`.

PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
Revert "fix(ngcc): do not spawn unlocker processes on cluster workers (…
06a9809 
…#36569)" (#36637)

This reverts commit 66effde.

PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
fix(ngcc): display unlocker process output in sync mode (#36637)
da159bd 
The change in e041ac6
to support sending unlocker process output to the main ngcc
console output prevents messages require that the main process
relinquishes the event-loop to allow the `stdout.on()` handler to
run.  This results in none of the messages being written when ngcc
is run in `--no-async` mode, and some messages failing to be
written if the main process is killed (e.g. ctrl-C).

It appears that the problem with Windows and detached processes
is known - see nodejs/node#3596 (comment).
But in the meantime, this commit is a workaround, where non-Windows
`inherit` the main process `stdout` while on Windows it reverts
to the async handler approach, which is better than nothing.

PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
refactor(ngcc): move command line option parsing to its own file (#36637
ed2b0e9 
)

PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
refactor(ngcc): move analyze and compile functions into their own fil…
bb7edc5 
…es (#36637)

PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
refactor(ngcc): move pathMapping processing to utils (#36637)
0075078 
PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
refactor(ngcc): create new entry-point for cluster workers (#36637)
506beed 
PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
refactor(ngcc): simplify cluster PackageJsonUpdater (#36637)
52aab63 
PR Close #36637
matsko pushed a commit that referenced this pull request Apr 16, 2020
@petebacondarwin @matsko
refactor(ngcc): moved shared setup into a single function (#36637)
479a59b 
The `main.ts` and `worker.ts` had duplicate logic, which has now been
moved to a single function called `getSharedSetup()`.

PR Close #36637
@petebacondarwin
petebacondarwin deleted the ngcc-worker-entry-point branch April 16, 2020 20:28
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented May 17, 2020

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators May 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@gkalpak gkalpak gkalpak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

3 participants

@petebacondarwin @gkalpak @googlebot