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

process: fix call process.reallyExit, vs., binding #25655

Closed
wants to merge 1 commit into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Jan 23, 2019

Some user-land modules, e.g., nyc, mocha, currently rely on patching
process.reallyExit.


Node@11.7.0 breaks the interaction between nyc and mocha; ultimately it appears as though the problem is that nyc relies on overriding process.reallyExit to output coverage to disk, which was replaced with binding.reallyExit (dde7152).

fixes: #25650

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

@bcoe bcoe added process Issues and PRs related to the process subsystem. regression Issues related to regressions. labels Jan 23, 2019
@bcoe
Copy link
Contributor Author

bcoe commented Jan 23, 2019

@bcoe
Copy link
Contributor Author

bcoe commented Jan 23, 2019

all thanks goes to @addaleax for this patch.

@boneskull
Copy link
Contributor

👍 for fix and test

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.

LGTM with a suggestion

lib/internal/process/per_thread.js Show resolved Hide resolved
@MylesBorins
Copy link
Contributor

Should we fast track this to get the fix for 11.7.0 out asap?

@addaleax addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 23, 2019
Some user-land modules, e.g., nyc, mocha, currently rely on patching
process.reallyExit.
@bcoe
Copy link
Contributor Author

bcoe commented Jan 23, 2019

@MylesBorins looks like folks are okay with fast-track, I will land this as soon as the smoke test finishes with @joyeecheung's copy edits.

bcoe added a commit to bcoe/node-1 that referenced this pull request Jan 23, 2019
Some user-land modules, e.g., nyc, mocha, currently rely on patching
process.reallyExit.

PR-URL: nodejs#25655
Fixes: nodejs#25650
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@bcoe
Copy link
Contributor Author

bcoe commented Jan 23, 2019

Landed in a6286e6

@bcoe bcoe closed this Jan 23, 2019
@bcoe bcoe deleted the fix-25650 branch January 23, 2019 18:21
@bcoe
Copy link
Contributor Author

bcoe commented Jan 23, 2019

@MylesBorins landed \o/ need to run to coffee, perhaps someone else can cherry-pick it to 11.7?

addaleax pushed a commit that referenced this pull request Jan 23, 2019
Some user-land modules, e.g., nyc, mocha, currently rely on patching
process.reallyExit.

PR-URL: #25655
Fixes: #25650
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. process Issues and PRs related to the process subsystem. regression Issues related to regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

user-land modules failing to hook process exit
9 participants