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

deps: add missing HandleScope in FieldType::PrintTo #22890

Closed
wants to merge 1 commit into from
Closed

deps: add missing HandleScope in FieldType::PrintTo #22890

wants to merge 1 commit into from

Conversation

hashseed
Copy link
Member

@hashseed hashseed commented Sep 17, 2018 •
edited
Loading

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Sep 17, 2018
@hashseed hashseed mentioned this pull request Sep 17, 2018
Closed
@BridgeAR
Copy link
Member

V8-CI https://ci.nodejs.org/job/node-test-commit-v8-linux/1691/
CI https://ci.nodejs.org/job/node-test-pull-request/17237/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 17, 2018
addaleax
addaleax approved these changes Sep 17, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

@hashseed Hope it’s okay to ask… Is it always okay to “just” add a HandleScope, or can V8 be in a state where that (or allocating handles in general) is not allowed?

@Trott
Copy link
Member

Trott commented Sep 17, 2018

Given the narrow scope of the change, would it make sense to fast track this to get CI back to green?

@addaleax addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Sep 17, 2018
Trott
Trott reviewed Sep 17, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Fixes the test that's been failing, so I'm 👍. Since this is a change in deps/v8/src, do we have to do anything special to make sure the patch gets re-floated when we update? (I guess that question may be for @nodejs/v8-update.)

@addaleax
Copy link
Member

@Trott As per the discussion in #22775, I don’t think we need to re-float this after updating to V8 7.0?

@hashseed
Copy link
Member Author

@Trott no need to refloat. It has been fixed as a byproduct of a refactoring.

@addaleax creating handle scopes and handles only hurts if done in a loop, it could slow down performance. You can't create handles outside the main thread of course. Also a handle scopes also limits handles created inside it to that scope.

@Trott
Copy link
Member

Trott commented Sep 17, 2018

Resume build: https://ci.nodejs.org/job/node-test-pull-request/17258/

I think arm-fanned doesn't play well with Resume Build so that one may need a Rebuild... Anyway, all failures look unrelated to this change, of course (and mostly build/infra related).

@refack
Copy link
Contributor

refack commented Sep 17, 2018

Resume 2: https://ci.nodejs.org/job/node-test-pull-request/17263/

@hashseed
Copy link
Member Author

Landed in dafaa6e.

hashseed added a commit that referenced this pull request Sep 18, 2018
@hashseed
deps: add missing HandleScope in FieldType::PrintTo
dafaa6e 
Refs: #22775

PR-URL: #22890
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax
Copy link
Member

Closing, since this has landed :)

@addaleax addaleax closed this Sep 18, 2018
@addaleax addaleax mentioned this pull request Sep 18, 2018
Closed
4 tasks
targos pushed a commit that referenced this pull request Sep 18, 2018
@hashseed @targos
deps: add missing HandleScope in FieldType::PrintTo
8ff23fe 
Refs: #22775

PR-URL: #22890
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@targos targos mentioned this pull request Sep 18, 2018
Merged
@joyeecheung joyeecheung mentioned this pull request Sep 18, 2018
Closed
targos pushed a commit that referenced this pull request Sep 19, 2018
@hashseed @targos
deps: add missing HandleScope in FieldType::PrintTo
6f66e49 
Refs: #22775

PR-URL: #22890
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Sep 20, 2018
@hashseed @targos
deps: add missing HandleScope in FieldType::PrintTo
bb3292f 
Refs: #22775

PR-URL: #22890
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@hashseed hashseed deleted the handlescopefix branch January 28, 2019 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@refack refack refack approved these changes

@addaleax addaleax addaleax approved these changes

@BridgeAR BridgeAR BridgeAR 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. build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hashseed @nodejs-github-bot @BridgeAR @Trott @addaleax @refack