Skip to content

Conversation

@thatshailesh
Copy link
Contributor

@thatshailesh thatshailesh commented May 25, 2018

on process exit if some assertion error occurs value of process._exiting
was hidden, this fix will show the actual error message with value

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 25, 2018
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think the comment is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, Actually this is my first PR, and not sure why windows test failed

I am a mac user, is there anything I can do to fix it?

Copy link
Member

Choose a reason for hiding this comment

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

The failure is unrelated, don't worry about that.

@lpinca
Copy link
Member

lpinca commented May 25, 2018

@thatshailesh thatshailesh requested a review from a team as a code owner May 25, 2018 07:45
@BridgeAR
Copy link
Member

@thatshailesh would you be so kind and rebase? Seems like you accidentally added the latest commit and that has to be backed out again.

@thatshailesh
Copy link
Contributor Author

@BridgeAR Sorry, I just did rebase, now only my commits are showing

Copy link
Member

Choose a reason for hiding this comment

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

How about this?:

process.nextTick(common.mustNotCall('process is exiting, should not be called'));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Trott
Copy link
Member

Trott commented May 28, 2018

Looks like you may have accidentally included a package-lock.json update too? You can remove that change, I would think, or whoever is landing this can remove it. (The removed change will probably be the same change proposed in #20970.)

on process exit if some assertion error occurs value of `process._exiting`
was hidden, this fix will show the actual error message with value
@thatshailesh
Copy link
Contributor Author

@Trott ok I have removed it, thanks 👍

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 28, 2018
@apapirovski
Copy link
Contributor

apapirovski commented Jun 1, 2018

Landed in 8055bdb

Congrats on your first contribution @thatshailesh!

@apapirovski apapirovski closed this Jun 1, 2018
apapirovski pushed a commit that referenced this pull request Jun 1, 2018
On process exit if some assertion error occurs value of
`process._exiting` was hidden, this fix will show the actual
error message with value.

PR-URL: #20956
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@thatshailesh
Copy link
Contributor Author

@apapirovski @Trott Thanks for the help 👍

addaleax pushed a commit that referenced this pull request Jun 1, 2018
On process exit if some assertion error occurs value of
`process._exiting` was hidden, this fix will show the actual
error message with value.

PR-URL: #20956
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jun 6, 2018
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. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants