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: refactor Environment::GetCurrent(isolate) usage #26376

Closed
wants to merge 5 commits into from
Closed

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

wants to merge 5 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Mar 1, 2019

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
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).
@addaleax
src: prefer to get Environment from Context
d85419d 
We explicitly store the context anyway, and can skip the
extra steps introduced in `Environment::GetCurrent()`.
@addaleax
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`.
@addaleax
src: forbid handle allocations from Platform tasks
6b766af 
Platform tasks should have their own handle scopes, rather than
leak into outer ones.
@nodejs-github-bot
Copy link
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 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
@addaleax
Copy link
Member Author

addaleax commented Mar 1, 2019

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

bnoordhuis
bnoordhuis approved these changes Mar 1, 2019
View reviewed changes
src/node_perf.cc Outdated Show resolved Hide resolved
@addaleax
Copy link
Member Author

addaleax commented Mar 4, 2019

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 4, 2019
@addaleax
Copy link
Member Author

addaleax commented Mar 5, 2019

Landed in cc4e8e0...820ae61

@addaleax addaleax closed this Mar 5, 2019
@addaleax addaleax deleted the embedding-env-getcurrent branch March 5, 2019 21:50
addaleax added a commit that referenced this pull request Mar 5, 2019
@addaleax
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Mar 5, 2019
@addaleax
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Mar 5, 2019
@addaleax
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Mar 5, 2019
@addaleax
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
@addaleax @BridgeAR
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
@addaleax @BridgeAR
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
@addaleax @BridgeAR
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
@addaleax @BridgeAR
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Mar 13, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZYSzys ZYSzys ZYSzys approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
@addaleax @nodejs-github-bot @bnoordhuis @jasnell @cjihrig @gireeshpunathil @ZYSzys