FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: refactor `Environment::GetCurrent(isolate)` usage by addaleax · Pull Request #26376 · nodejs/node · GitHub

/ node Public

src: refactor Environment::GetCurrent(isolate) usage - #26376

Closed
addaleax wants to merge 5 commits into
nodejs:masterfrom
addaleax:embedding-env-getcurrent
ConversationCommits (5)ChecksFiles changed
Closed

src: refactor Environment::GetCurrent(isolate) usage#26376
addaleax wants to merge 5 commits into
nodejs:masterfrom
addaleax:embedding-env-getcurrent

Conversation

addaleax commented Mar 1, 2019

Copy link
Copy Markdown
Member
src: refactor Environment::GetCurrent(isolate) usage

Do not require an explicit HandleScope, or the ability to create
one, when using Environment::GetCurrent().

isolate->InContext() is used as an indicator that it is probably
okay to create a HandleScope, see also the short discussion in
#25775 (review).

src: prefer to get Environment from Context

We explicitly store the context anyway, and can skip the
extra steps introduced in Environment::GetCurrent().

src: allow running tasks without Environment

There is no real reason to assume that V8 tasks would have
to run in a Node.js Context.

src: forbid handle allocations from Platform tasks

Platform tasks should have their own handle scopes, rather than
leak into outer ones.

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

addaleax added 4 commits March 1, 2019 16:32
src: refactor Environment::GetCurrent(isolate) usage
3509d74 
Do not require an explicit `HandleScope`, or the ability to create
one, when using `Environment::GetCurrent()`.

`isolate->InContext()` is used as an indicator that it is probably
okay to create a `HandleScope`, see also the short discussion in
nodejs#25775 (review).
src: prefer to get Environment from Context
d85419d 
We explicitly store the context anyway, and can skip the
extra steps introduced in `Environment::GetCurrent()`.
src: allow running tasks without Environment
322041b 
There is no real reason to assume that V8 tasks would have
to run in a Node.js `Context`.
src: forbid handle allocations from Platform tasks
6b766af 
Platform tasks should have their own handle scopes, rather than
leak into outer ones.
addaleax requested a review from gireeshpunathil March 1, 2019 15:49

nodejs-github-bot commented Mar 1, 2019

Copy link
Copy Markdown
Collaborator

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

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 Mar 1, 2019
gireeshpunathil reviewed Mar 1, 2019
View reviewed changes
Comment thread src/env-inl.h

addaleax commented Mar 1, 2019

Copy link
Copy Markdown
Member Author

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

ZYSzys reviewed Mar 1, 2019
View reviewed changes
Comment thread src/node_platform.cc
bnoordhuis approved these changes Mar 1, 2019
View reviewed changes
Comment thread src/node_perf.cc Outdated

addaleax commented Mar 4, 2019

Copy link
Copy Markdown
Member Author

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

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 4, 2019

addaleax commented Mar 5, 2019

Copy link
Copy Markdown
Member Author

Landed in cc4e8e0...820ae61

addaleax closed this Mar 5, 2019
addaleax deleted the embedding-env-getcurrent branch March 5, 2019 21:50
addaleax added a commit that referenced this pull request Mar 5, 2019
src: refactor Environment::GetCurrent(isolate) usage
c2f620a 
Do not require an explicit `HandleScope`, or the ability to create
one, when using `Environment::GetCurrent()`.

`isolate->InContext()` is used as an indicator that it is probably
okay to create a `HandleScope`, see also the short discussion in
#25775 (review).

PR-URL: #26376
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Mar 5, 2019
src: prefer to get Environment from Context
cf699a0 
We explicitly store the context anyway, and can skip the
extra steps introduced in `Environment::GetCurrent()`.

PR-URL: #26376
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Mar 5, 2019
src: allow running tasks without Environment
753ebd7 
There is no real reason to assume that V8 tasks would have
to run in a Node.js `Context`.

PR-URL: #26376
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Mar 5, 2019
src: forbid handle allocations from Platform tasks
820ae61 
Platform tasks should have their own handle scopes, rather than
leak into outer ones.

PR-URL: #26376
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
src: refactor Environment::GetCurrent(isolate) usage
716ec00 
Do not require an explicit `HandleScope`, or the ability to create
one, when using `Environment::GetCurrent()`.

`isolate->InContext()` is used as an indicator that it is probably
okay to create a `HandleScope`, see also the short discussion in
nodejs#25775 (review).

PR-URL: nodejs#26376
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
src: prefer to get Environment from Context
622048d 
We explicitly store the context anyway, and can skip the
extra steps introduced in `Environment::GetCurrent()`.

PR-URL: nodejs#26376
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
src: allow running tasks without Environment
9c277c0 
There is no real reason to assume that V8 tasks would have
to run in a Node.js `Context`.

PR-URL: nodejs#26376
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
src: forbid handle allocations from Platform tasks
170e196 
Platform tasks should have their own handle scopes, rather than
leak into outer ones.

PR-URL: nodejs#26376
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR mentioned this pull request Mar 13, 2019
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
gireeshpunathil gireeshpunathil approved these changes
ZYSzys ZYSzys approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL