Skip to content

test: forbid common.mustCall*() in process exit handlers#17453

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:mustcall-in-exit
Closed

test: forbid common.mustCall*() in process exit handlers#17453
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:mustcall-in-exit

Conversation

@Trott
Copy link
Copy Markdown
Member

@Trott Trott commented Dec 4, 2017

common.mustCall() and common.mustCallAtLeast() need to be called
before process exit handlers to work because checks are done inside a
process exit handler. Detect if being used inside a process exit handler
and throw.

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

test

`common.mustCall()` and `common.mustCallAtLeast()` need to be called
before process exit handlers to work because checks are done inside a
process exit handler. Detect if being used inside a process exit handler
and throw.
@Trott Trott added the test Issues and PRs related to the tests. label Dec 4, 2017
Copy link
Copy Markdown
Contributor

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

LGTM — I take it we don't have any tests currently that have an issue with this?

@Trott Trott mentioned this pull request Dec 4, 2017
4 tasks
@Trott
Copy link
Copy Markdown
Member Author

Trott commented Dec 4, 2017

LGTM — I take it we don't have any tests currently that have an issue with this?

@apapirovski make -j4 test passed for me, so a qualified "yes". (Qualified because make test doesn't run all the test suites, and maybe there are skipped tests on my local platform, etc.)

@Trott
Copy link
Copy Markdown
Member Author

Trott commented Dec 5, 2017

@Trott
Copy link
Copy Markdown
Member Author

Trott commented Dec 5, 2017

CI failure appears completely unrelated, but just in case, here's a re-run of test/linux-openssl110:

https://ci.nodejs.org/job/node-test-commit-linux-linked/598/

Trott added a commit to Trott/io.js that referenced this pull request Dec 5, 2017
`common.mustCall()` and `common.mustCallAtLeast()` need to be called
before process exit handlers to work because checks are done inside a
process exit handler. Detect if being used inside a process exit handler
and throw.

PR-URL: nodejs#17453
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Copy Markdown
Member Author

Trott commented Dec 5, 2017

Landed in 591a24b

@Trott Trott closed this Dec 5, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
`common.mustCall()` and `common.mustCallAtLeast()` need to be called
before process exit handlers to work because checks are done inside a
process exit handler. Detect if being used inside a process exit handler
and throw.

PR-URL: #17453
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
`common.mustCall()` and `common.mustCallAtLeast()` need to be called
before process exit handlers to work because checks are done inside a
process exit handler. Detect if being used inside a process exit handler
and throw.

PR-URL: #17453
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
`common.mustCall()` and `common.mustCallAtLeast()` need to be called
before process exit handlers to work because checks are done inside a
process exit handler. Detect if being used inside a process exit handler
and throw.

PR-URL: #17453
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
`common.mustCall()` and `common.mustCallAtLeast()` need to be called
before process exit handlers to work because checks are done inside a
process exit handler. Detect if being used inside a process exit handler
and throw.

PR-URL: #17453
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
@Trott Trott deleted the mustcall-in-exit branch January 13, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Issues and PRs related to the tests.

7 participants