Skip to content
This repository has been archived by the owner on May 8, 2018. It is now read-only.

Use process.exitCode instead of process.exit() #219

Closed
wants to merge 1 commit into from

Conversation

blicksky
Copy link

This pull request replaces calls to process.exit() with assignments to process.exitCode. This is necessary because calling process.exit() forcibly terminates the process without waiting for streams, including stdout and stderr, to be flushed.

From https://nodejs.org/api/process.html#process_process_exit_code:

It is important to note that calling process.exit() will force the process to exit as quickly as possible even if there are still asynchronous operations pending that have not yet completed fully, including I/O operations to process.stdout and process.stderr.

This caused me problems when attempting to spawn nsp as a child process and read the result of the JSON reporter from stdout. The output was always truncated at 8192, and some projects produced output larger than that.

See nodejs/node#12921.

@blicksky
Copy link
Author

The tests pass on my machine. It seems that the CI failure is related to finding a .nsprc file in a home directory outside of the project repository, which is out of my control as far as I can tell.

@blicksky
Copy link
Author

Fixes #248

Copy link

@KevinHock KevinHock left a comment

Choose a reason for hiding this comment

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

This should be merged :D Thank you @blicksky for making this.

@KevinHock
Copy link

Hi @nlf @wraithgar @evilpacket, can you please merge this PR when you get a chance?

@nlf
Copy link
Member

nlf commented May 8, 2018

The Node Security Platform has been acquired by npm, Inc.
On April 10, 2018, the Node Security Platform joined npm, Inc., which operates the npm JavaScript package registry.

Learn more here.

The service remains operational for current users in its current state. No new features or fixes will be implemented.

@nlf nlf closed this May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants