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.

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

src: cleanup in all return paths in node::Start #26471

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

gireeshpunathil
Copy link
Member

node::Start creates a number of artifacts in its scope which are
cleaned up in the exit path, but there is at least one path where the
cleanups are bypassed. Force all paths follow the exit sequence.

Refs: #21283

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 6, 2019
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Makes you wonder maybe this should be some kind of scope...but then it would be nice if the Node instance can be RAII

@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2019

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

unfortunately the exit_code was declared under #ifdef HAVE_INSPECTOR block that caused build failure in non-standard jobs (--without-intl, --without-ssl, --without-inspector...)

fixed by elevating the scope at function level.

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

@gireeshpunathil
Copy link
Member Author

gireeshpunathil commented Mar 7, 2019

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/21289/ (:heavy_check_mark:)

`node::Start` creates a number of artifacts in its scope which are
cleaned up in the exit path, but there is at least one path where the
cleanups are bypassed. Force all paths follow the exit sequence.

Refs: nodejs#21283
PR-URL: nodejs#26471
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@gireeshpunathil gireeshpunathil removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 8, 2019
@gireeshpunathil
Copy link
Member Author

landed as e029bc9

@gireeshpunathil gireeshpunathil merged commit e029bc9 into nodejs:master Mar 8, 2019
@gireeshpunathil gireeshpunathil mentioned this pull request Mar 8, 2019
4 tasks
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
`node::Start` creates a number of artifacts in its scope which are
cleaned up in the exit path, but there is at least one path where the
cleanups are bypassed. Force all paths follow the exit sequence.

Refs: nodejs#21283
PR-URL: nodejs#26471
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
`node::Start` creates a number of artifacts in its scope which are
cleaned up in the exit path, but there is at least one path where the
cleanups are bypassed. Force all paths follow the exit sequence.

Refs: #21283
PR-URL: #26471
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants