src: pass context to Get() operations for child processes by cjihrig · Pull Request #16247 · nodejs/node

cjihrig · 2017-10-17T04:03:04Z

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

TimothyGu · 2017-10-17T04:24:58Z

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

Yes it does. Updated.

joyeecheung · 2017-10-17T05:53:48Z

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

bnoordhuis

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

cjihrig · 2017-10-18T18:08:24Z

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

cjihrig · 2017-10-18T19:08:27Z

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

Refs: nodejs#15380 PR-URL: nodejs#16247 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

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]>

MylesBorins · 2017-10-23T22:14:20Z

this does not land cleanly on 8.x

Would you be willing to backport @cjihrig ?

Refs: nodejs#15380 PR-URL: nodejs#16247 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 · 2017-10-24T01:20:06Z

Backport in #16426

Backport-PR-URL: #16426 Refs: #15380 PR-URL: #16247 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

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]>

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

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 · 2017-11-16T21:04:38Z

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.

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

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]>

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 reviewed Oct 17, 2017

View reviewed changes

cjihrig force-pushed the context branch from 251f7cd to f012a5a Compare October 17, 2017 15:53

bnoordhuis approved these changes Oct 17, 2017

View reviewed changes

jasnell approved these changes Oct 18, 2017

View reviewed changes

cjihrig force-pushed the context branch from f012a5a to 4a86c40 Compare October 18, 2017 18:06

cjihrig added 3 commits October 18, 2017 16:38

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]>

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 force-pushed the context branch from 4a86c40 to 6dcc37d Compare October 18, 2017 20:38

cjihrig merged commit 6dcc37d into nodejs:master Oct 18, 2017

cjihrig deleted the context branch October 18, 2017 20:40

MylesBorins added the backport-requested-v8.x label Oct 23, 2017

cjihrig added a commit to cjihrig/node that referenced this pull request Oct 24, 2017

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

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 mentioned this pull request Oct 24, 2017

[v8.x backport] src: pass context to Get() operations for child processes #16426

Closed

4 tasks

MylesBorins pushed a commit that referenced this pull request Oct 24, 2017

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

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]>

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017

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

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]>

lance added backported-to-v8.x and removed backport-requested-v8.x labels Oct 28, 2017

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

addaleax mentioned this pull request Nov 28, 2017

src: replace ->To*(isolate) with ->To*(context).ToLocalChecked() #17343

Closed

2 tasks

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017

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

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]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

src: pass context to Get() operations for child processes#16247
cjihrig merged 3 commits intonodejs:masterfrom
cjihrig:context

cjihrig commented Oct 17, 2017

Uh oh!

TimothyGu Oct 17, 2017

Uh oh!

cjihrig Oct 17, 2017

Uh oh!

joyeecheung commented Oct 17, 2017

Uh oh!

bnoordhuis left a comment

Uh oh!

cjihrig commented Oct 18, 2017

Uh oh!

cjihrig commented Oct 18, 2017

Uh oh!

MylesBorins commented Oct 23, 2017

Uh oh!

cjihrig commented Oct 24, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

cjihrig commented Oct 17, 2017

Checklist

Affected core subsystem(s)

Uh oh!

TimothyGu Oct 17, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig Oct 17, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Oct 17, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Oct 18, 2017

Uh oh!

cjihrig commented Oct 18, 2017

Uh oh!

MylesBorins commented Oct 23, 2017

Uh oh!

cjihrig commented Oct 24, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants