src: move more process methods initialization in bootstrap/node.js by joyeecheung · Pull Request #25127 · nodejs/node

joyeecheung · 2018-12-19T08:34:55Z

Instead of:

Writing methods onto the process directly in C++ during
SetupProcessObject() and overwrite with argument checks later
Or, wrapping and writing them in internal/process/*.js

Do:

Move the C++ implementations in node_process.cc and mark them static
wherever possible
Expose the C++ methods through a new
internalBinding('process_methods')
Wrap the methods in internal/process/*.js in a
side-effect-free manner and return them back to
internal/bootstrap/node.js
Centralize the write to the process object based on conditions
in bootstrap/node.js

So it's easier to see what methods are attached to the process object
during bootstrap under what condition and in what order.

The eventual goal is to figure out the dependency of process methods
and the write/read access to the process object during bootstrap, group
these access properly and remove the process properties that should not
be exposed to users this way.

Also correct the NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE milestone
which should be marked before code execution.

Refs: #24961

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2018-12-19T08:34:57Z

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2002/pipeline

joyeecheung · 2018-12-19T08:35:14Z

cc @addaleax @jasnell @nodejs/process
CI: https://ci.nodejs.org/job/node-test-commit/24486/

src/node_process.cc

joyeecheung · 2018-12-19T15:47:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/19682/

joyeecheung · 2018-12-19T15:47:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/19682/

joyeecheung · 2018-12-20T23:43:42Z

Resume: https://ci.nodejs.org/job/node-test-pull-request/19702/

Instead of: - Writing methods onto the process directly in C++ during `SetupProcessObject()` and overwrite with argument checks later - Or, wrapping and writing them in `internal/process/*.js` Do: - Move the C++ implementations in node_process.cc and mark them static wherever possible - Expose the C++ methods through a new `internalBinding('process_methods')` - Wrap the methods in `internal/process/*.js` in a side-effect-free manner and return them back to `internal/bootstrap/node.js` - Centralize the write to the process object based on conditions in `bootstrap/node.js` So it's easier to see what methods are attached to the process object during bootstrap under what condition and in what order. The eventual goal is to figure out the dependency of process methods and the write/read access to the process object during bootstrap, group these access properly and remove the process properties that should not be exposed to users this way. Also correct the NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE milestone which should be marked before code execution. Refs: nodejs#24961

joyeecheung · 2018-12-21T01:49:35Z

Rebased. CI: https://ci.nodejs.org/job/node-test-pull-request/19707/
Resume: https://ci.nodejs.org/job/node-test-pull-request/19712/

joyeecheung · 2018-12-21T08:45:25Z

Landed in 75d0a93, thanks!

Instead of: - Writing methods onto the process directly in C++ during `SetupProcessObject()` and overwrite with argument checks later - Or, wrapping and writing them in `internal/process/*.js` Do: - Move the C++ implementations in node_process.cc and mark them static wherever possible - Expose the C++ methods through a new `internalBinding('process_methods')` - Wrap the methods in `internal/process/*.js` in a side-effect-free manner and return them back to `internal/bootstrap/node.js` - Centralize the write to the process object based on conditions in `bootstrap/node.js` So it's easier to see what methods are attached to the process object during bootstrap under what condition and in what order. The eventual goal is to figure out the dependency of process methods and the write/read access to the process object during bootstrap, group these access properly and remove the process properties that should not be exposed to users this way. Also correct the NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE milestone which should be marked before code execution. Refs: #24961 PR-URL: #25127 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-12-25T06:51:08Z

This isn't landing cleanly on v11.x, should it be backported?

addaleax · 2019-01-05T21:46:32Z

@joyeecheung I think #25127 needs to be backported along with this?

joyeecheung · 2019-01-06T02:19:52Z

@addaleax Did you mean #25289 ? (and yes, I think so)

Instead of: - Writing methods onto the process directly in C++ during `SetupProcessObject()` and overwrite with argument checks later - Or, wrapping and writing them in `internal/process/*.js` Do: - Move the C++ implementations in node_process.cc and mark them static wherever possible - Expose the C++ methods through a new `internalBinding('process_methods')` - Wrap the methods in `internal/process/*.js` in a side-effect-free manner and return them back to `internal/bootstrap/node.js` - Centralize the write to the process object based on conditions in `bootstrap/node.js` So it's easier to see what methods are attached to the process object during bootstrap under what condition and in what order. The eventual goal is to figure out the dependency of process methods and the write/read access to the process object during bootstrap, group these access properly and remove the process properties that should not be exposed to users this way. Also correct the NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE milestone which should be marked before code execution. Refs: nodejs#24961 PR-URL: nodejs#25127 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR · 2019-01-15T15:19:50Z

@joyeecheung this seems to be the oldest PR which requires a manual backport. Could you have another look at this one? All other manual backport requests are about process changes and should rely on this PR.

Instead of: - Writing methods onto the process directly in C++ during `SetupProcessObject()` and overwrite with argument checks later - Or, wrapping and writing them in `internal/process/*.js` Do: - Move the C++ implementations in node_process.cc and mark them static wherever possible - Expose the C++ methods through a new `internalBinding('process_methods')` - Wrap the methods in `internal/process/*.js` in a side-effect-free manner and return them back to `internal/bootstrap/node.js` - Centralize the write to the process object based on conditions in `bootstrap/node.js` So it's easier to see what methods are attached to the process object during bootstrap under what condition and in what order. The eventual goal is to figure out the dependency of process methods and the write/read access to the process object during bootstrap, group these access properly and remove the process properties that should not be exposed to users this way. Also correct the NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE milestone which should be marked before code execution. Refs: #24961 PR-URL: #25127 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Backport-PR-URL: #25496

Instead of: - Writing methods onto the process directly in C++ during `SetupProcessObject()` and overwrite with argument checks later - Or, wrapping and writing them in `internal/process/*.js` Do: - Move the C++ implementations in node_process.cc and mark them static wherever possible - Expose the C++ methods through a new `internalBinding('process_methods')` - Wrap the methods in `internal/process/*.js` in a side-effect-free manner and return them back to `internal/bootstrap/node.js` - Centralize the write to the process object based on conditions in `bootstrap/node.js` So it's easier to see what methods are attached to the process object during bootstrap under what condition and in what order. The eventual goal is to figure out the dependency of process methods and the write/read access to the process object during bootstrap, group these access properly and remove the process properties that should not be exposed to users this way. Also correct the NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE milestone which should be marked before code execution. Refs: nodejs#24961 PR-URL: nodejs#25127 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Backport-PR-URL: nodejs#25496

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 19, 2018

addaleax approved these changes Dec 19, 2018

View reviewed changes

src/node_process.cc Outdated Show resolved Hide resolved

jasnell approved these changes Dec 19, 2018

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 20, 2018

joyeecheung force-pushed the move-process-methods branch from 9edfd9d to 7108077 Compare December 21, 2018 01:09

joyeecheung closed this Dec 21, 2018

MylesBorins added the backport-requested-v11.x label Dec 25, 2018

Trott mentioned this pull request Dec 25, 2018

src: fix compiler warnings #25165

Closed

2 tasks

bcoe mentioned this pull request Dec 30, 2018

NODE_V8_COVERAGE broken on OSX (perhaps other platforms) #25287

Closed

addaleax mentioned this pull request Jan 5, 2019

coverage: fix broken coverage setup #25289

Merged

2 tasks

addaleax mentioned this pull request Jan 14, 2019

[v11.x] src: move more process methods initialization in bootstrap/node.js #25496

Closed

addaleax added backported-to-v11.x and removed backport-requested-v11.x labels Jan 15, 2019

BridgeAR mentioned this pull request Jan 16, 2019

v11.7.0 proposal #25537

Merged

addaleax mentioned this pull request Jan 23, 2019

user-land modules failing to hook process exit #25650

Closed

MylesBorins mentioned this pull request Jan 24, 2019

v11.8.0 proposal #25687

Merged

BethGriggs added the backport-requested-v10.x label Jul 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

src: move more process methods initialization in bootstrap/node.js#25127

src: move more process methods initialization in bootstrap/node.js#25127
joyeecheung wants to merge 1 commit intonodejs:masterfrom
joyeecheung:move-process-methods

joyeecheung commented Dec 19, 2018

Uh oh!

nodejs-github-bot commented Dec 19, 2018

Uh oh!

joyeecheung commented Dec 19, 2018

Uh oh!

Uh oh!

joyeecheung commented Dec 19, 2018

Uh oh!

joyeecheung commented Dec 19, 2018

Uh oh!

joyeecheung commented Dec 20, 2018

Uh oh!

joyeecheung commented Dec 21, 2018 •

edited

Loading

Uh oh!

joyeecheung commented Dec 21, 2018

Uh oh!

MylesBorins commented Dec 25, 2018

Uh oh!

addaleax commented Jan 5, 2019

Uh oh!

joyeecheung commented Jan 6, 2019

Uh oh!

BridgeAR commented Jan 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

joyeecheung commented Dec 19, 2018

Checklist

Uh oh!

nodejs-github-bot commented Dec 19, 2018

Uh oh!

joyeecheung commented Dec 19, 2018

Uh oh!

Uh oh!

joyeecheung commented Dec 19, 2018

Uh oh!

joyeecheung commented Dec 19, 2018

Uh oh!

joyeecheung commented Dec 20, 2018

Uh oh!

joyeecheung commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Dec 21, 2018

Uh oh!

MylesBorins commented Dec 25, 2018

Uh oh!

addaleax commented Jan 5, 2019

Uh oh!

joyeecheung commented Jan 6, 2019

Uh oh!

BridgeAR commented Jan 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

joyeecheung commented Dec 21, 2018 •

edited

Loading