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: isolate->Dispose() order consistency #30181

Closed
wants to merge 1 commit into from
Closed

src: isolate->Dispose() order consistency #30181

wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Oct 30, 2019 •
edited
Loading

Removes comment from UnregisterIsolate() specifying that Isolate::Dispose() should be called after UnregisterIsolate() since that's no longer the case.

This was originally added to account for issues in V8 but now there should be no relevant tasks running during either call so their order is fungible. However, for consistency's sake, we were calling most of these in the same order (dispose and then unregister) so this also moves a straggling call from node_worker to bring it in line with all others.

cc @addaleax

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

@codebytere codebytere requested a review from addaleax October 30, 2019 17:57
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support. labels Oct 30, 2019
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 30, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@codebytere
Copy link
Member Author

Windows Rerun: https://ci.nodejs.org/job/node-test-commit-windows-fanned/30837/

@codebytere
Copy link
Member Author

Landed in 312c02d

@codebytere codebytere closed this Nov 1, 2019
@codebytere codebytere deleted the consistent-dispose branch November 1, 2019 21:10
codebytere added a commit that referenced this pull request Nov 1, 2019
@codebytere
src: isolate->Dispose() order consistency
312c02d 
PR-URL: #30181
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
targos pushed a commit that referenced this pull request Nov 5, 2019
@codebytere @targos
src: isolate->Dispose() order consistency
cc1cd2b 
PR-URL: #30181
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@targos targos mentioned this pull request Nov 5, 2019
Merged
targos pushed a commit that referenced this pull request Nov 8, 2019
@codebytere @targos
src: isolate->Dispose() order consistency
77861dd 
PR-URL: #30181
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
@codebytere @targos
src: isolate->Dispose() order consistency
7b3baab 
PR-URL: #30181
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
@codebytere @targos
src: isolate->Dispose() order consistency
3b6a7c5 
PR-URL: #30181
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@targos targos mentioned this pull request Nov 10, 2019
Merged
targos pushed a commit that referenced this pull request Nov 11, 2019
@codebytere @targos
src: isolate->Dispose() order consistency
ab03c29 
PR-URL: #30181
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@addaleax addaleax mentioned this pull request Dec 11, 2019
Closed
addaleax added a commit to addaleax/node that referenced this pull request Dec 12, 2019
@addaleax
src: unregister Isolate with platform before disposing
27d48ed 
I previously thought the order of these calls was no longer
relevant. I was wrong.

This commit undoes the changes from 312c02d, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d.

Fixes: nodejs#30846
Refs: nodejs#30181
@addaleax addaleax mentioned this pull request Dec 12, 2019
Closed
2 tasks
Trott pushed a commit that referenced this pull request Dec 14, 2019
@addaleax @Trott
src: unregister Isolate with platform before disposing
25447d8 
I previously thought the order of these calls was no longer
relevant. I was wrong.

This commit undoes the changes from 312c02d, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d.

Fixes: #30846
Refs: #30181

PR-URL: #30909
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
@addaleax @MylesBorins
src: unregister Isolate with platform before disposing
d456aa0 
I previously thought the order of these calls was no longer
relevant. I was wrong.

This commit undoes the changes from 312c02d, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d.

Fixes: #30846
Refs: #30181

PR-URL: #30909
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
@addaleax @targos
src: unregister Isolate with platform before disposing
ace3e70 
I previously thought the order of these calls was no longer
relevant. I was wrong.

This commit undoes the changes from 312c02d, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d.

Fixes: #30846
Refs: #30181

PR-URL: #30909
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@addaleax @BethGriggs
src: unregister Isolate with platform before disposing
65e5a8a 
I previously thought the order of these calls was no longer
relevant. I was wrong.

This commit undoes the changes from 312c02d, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d.

Fixes: #30846
Refs: #30181

PR-URL: #30909
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@addaleax addaleax added the v8 platform Issues and PRs related to Node's v8::Platform implementation. label Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek approved these changes

@devnexen devnexen devnexen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax 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++. v8 platform Issues and PRs related to Node's v8::Platform implementation. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@codebytere @nodejs-github-bot @addaleax @cjihrig @devnexen @devsnek