Skip to content

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

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
Merged
cjihrig merged 3 commits into from
Oct 18, 2017
Merged

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

cjihrig merged 3 commits into from
Oct 18, 2017

Conversation

@cjihrig
Copy link
Contributor

@cjihrigcjihrig 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-botnodejs-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
Copy link
Member

@TimothyGuTimothyGuOct 17, 2017

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
ContributorAuthor

@cjihrigcjihrigOct 17, 2017

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

joyeecheung commented Oct 17, 2017

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

@bnoordhuisbnoordhuis 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
ContributorAuthor

cjihrig commented Oct 18, 2017

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

@cjihrig
Copy link
ContributorAuthor

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]>
@cjihrigcjihrig merged commit 6dcc37d into nodejs:masterOct 18, 2017
@cjihrigcjihrig deleted the context branch October 18, 2017 20:40
@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 23, 2017

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]>
@cjihrigcjihrig mentioned this pull request Oct 24, 2017
Closed
4 tasks
@cjihrig
Copy link
ContributorAuthor

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

MylesBorins commented Nov 16, 2017

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.

@addaleaxaddaleax 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes

@jasnelljasnelljasnell approved these changes

+1 more reviewer

@TimothyGuTimothyGuTimothyGu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.child_processIssues 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