Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: pass context to Get() operations for child processes #16247

Merged
merged 3 commits into from
Oct 18, 2017
Merged

src: pass context to Get() operations for child processes #16247

merged 3 commits into from
Oct 18, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Oct 17, 2017

Replace Get(key) with Get(context, key) in src/process_wrap.cc and src/spawn_sync.cc.

cc: @bnoordhuis per #15380 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. labels Oct 17, 2017
TimothyGu
TimothyGu reviewed Oct 17, 2017
View reviewed changes
src/process_wrap.cc Outdated
@@ -93,39 +93,46 @@ class ProcessWrap : public HandleWrap {
static void ParseStdioOptions(Environment* env,
Local<Object> js_options,
uv_process_options_t* options) {
Local<Context> context = env->isolate()->GetCurrentContext();
Copy link
Member

Choose a reason for hiding this comment

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

Does env->context() work? Ditto below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it does. Updated.

@joyeecheung
Copy link
Member

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

bnoordhuis
bnoordhuis approved these changes Oct 17, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. While you're at it, you might also want to update the BooleanValue(), IntegerValue() and Int32Value() calls.

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 18, 2017

New CI with requested changes: https://ci.nodejs.org/job/node-test-pull-request/10819/

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 18, 2017

CI failures were unrelated. Looks like #16208 (comment), a known flake, and a Jenkins issue.

@cjihrig
src: increase usage of context in process_wrap.cc
efde153 
Refs: nodejs#15380
PR-URL: nodejs#16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig
src: increase usage of context in spawn_sync.cc
ad07065 
Refs: nodejs#15380
PR-URL: nodejs#16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig
src: combine loops in CopyJsStringArray()
6dcc37d 
In spawn_sync.cc, two consecutive loops are used to convert
data to strings, and compute the size of the data. This commit
merges the two independent loops into one.

Refs: nodejs#15380
PR-URL: nodejs#16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig merged commit 6dcc37d into nodejs:master Oct 18, 2017
@cjihrig cjihrig deleted the context branch October 18, 2017 20:40
@MylesBorins
Copy link
Contributor

this does not land cleanly on 8.x

Would you be willing to backport @cjihrig ?

cjihrig added a commit to cjihrig/node that referenced this pull request Oct 24, 2017
@cjihrig
src: increase usage of context in process_wrap.cc
e8cbacd 
Refs: nodejs#15380
PR-URL: nodejs#16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this pull request Oct 24, 2017
@cjihrig
src: increase usage of context in spawn_sync.cc
4af1bec 
Refs: nodejs#15380
PR-URL: nodejs#16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this pull request Oct 24, 2017
@cjihrig
src: combine loops in CopyJsStringArray()
c4f2898 
In spawn_sync.cc, two consecutive loops are used to convert
data to strings, and compute the size of the data. This commit
merges the two independent loops into one.

Refs: nodejs#15380
PR-URL: nodejs#16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig mentioned this pull request Oct 24, 2017
Closed
4 tasks
@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 24, 2017

Backport in #16426

MylesBorins pushed a commit that referenced this pull request Oct 24, 2017
@cjihrig @MylesBorins
src: increase usage of context in process_wrap.cc
91dc6c7 
Backport-PR-URL: #16426
Refs: #15380
PR-URL: #16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 24, 2017
@cjihrig @MylesBorins
src: increase usage of context in spawn_sync.cc
5dbefe1 
Backport-PR-URL: #16426
Refs: #15380
PR-URL: #16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 24, 2017
@cjihrig @MylesBorins
src: combine loops in CopyJsStringArray()
78a6ef4 
In spawn_sync.cc, two consecutive loops are used to convert
data to strings, and compute the size of the data. This commit
merges the two independent loops into one.

Backport-PR-URL: #16426
Refs: #15380
PR-URL: #16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@cjihrig @addaleax
src: increase usage of context in process_wrap.cc
db1b343 
Refs: nodejs/node#15380
PR-URL: nodejs/node#16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@cjihrig @addaleax
src: increase usage of context in spawn_sync.cc
9934df1 
Refs: nodejs/node#15380
PR-URL: nodejs/node#16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@cjihrig @addaleax
src: combine loops in CopyJsStringArray()
760d5e9 
In spawn_sync.cc, two consecutive loops are used to convert
data to strings, and compute the size of the data. This commit
merges the two independent loops into one.

Refs: nodejs/node#15380
PR-URL: nodejs/node#16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@addaleax addaleax mentioned this pull request Nov 28, 2017
Closed
2 tasks
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@cjihrig @addaleax
src: increase usage of context in process_wrap.cc
8efc0f9 
Refs: nodejs/node#15380
PR-URL: nodejs/node#16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@cjihrig @addaleax
src: increase usage of context in spawn_sync.cc
f895e1c 
Refs: nodejs/node#15380
PR-URL: nodejs/node#16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@cjihrig @addaleax
src: combine loops in CopyJsStringArray()
c8ca027 
In spawn_sync.cc, two consecutive loops are used to convert
data to strings, and compute the size of the data. This commit
merges the two independent loops into one.

Refs: nodejs/node#15380
PR-URL: nodejs/node#16247
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cjihrig @joyeecheung @MylesBorins @bnoordhuis @jasnell @TimothyGu @lance @nodejs-github-bot