Skip to content

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

Closed
hashseed wants to merge 1 commit intonodejs:masterfrom
hashseed:handlescopefix
Closed

deps: add missing HandleScope in FieldType::PrintTo#22890
hashseed wants to merge 1 commit intonodejs:masterfrom
hashseed:handlescopefix

Conversation

@hashseed
Copy link
Copy Markdown
Member

@hashseed hashseed commented Sep 17, 2018 •
edited
Loading

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Sep 17, 2018

@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

@hashseed hashseed requested review from Trott and addaleax September 17, 2018 05:46
@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
Copy Markdown
Member

BridgeAR commented Sep 17, 2018

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
Copy Markdown
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
Copy Markdown
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 72 hours to land. label Sep 17, 2018
Trott
Trott reviewed Sep 17, 2018
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

addaleax commented Sep 17, 2018

@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
Copy Markdown
Member Author

hashseed commented Sep 17, 2018

@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
Copy Markdown
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
Copy Markdown
Contributor

refack commented Sep 17, 2018

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

@hashseed
Copy link
Copy Markdown
Member Author

hashseed commented Sep 18, 2018

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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@addaleax
Copy link
Copy Markdown
Member

addaleax commented Sep 18, 2018

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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@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

@addaleax addaleax addaleax approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

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