Skip to content

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

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

nodejs-github-bot commented Oct 30, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/26248/
@codebytere
Copy link
Member Author

codebytere commented Nov 1, 2019

Windows Rerun: https://ci.nodejs.org/job/node-test-commit-windows-fanned/30837/
@codebytere
Copy link
Member Author

codebytere commented Nov 1, 2019

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 <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: David Carlier <devnexen@gmail.com>
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 <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: David Carlier <devnexen@gmail.com>
@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 <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: David Carlier <devnexen@gmail.com>
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 <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: David Carlier <devnexen@gmail.com>
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 <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: David Carlier <devnexen@gmail.com>
@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 <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: David Carlier <devnexen@gmail.com>
@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 <codebytere@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <codebytere@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <codebytere@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <codebytere@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
@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

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.

6 participants

@codebytere @nodejs-github-bot @addaleax @cjihrig @devnexen @devsnek