Skip to content

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Oct 27, 2017

Move the throw out of c++ and into js using internal/errors

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
Affected core subsystem(s)

src, repl

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module. labels Oct 27, 2017
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 27, 2017
@jasnell
Copy link
Member Author

jasnell commented Oct 27, 2017

Ping @nodejs/tsc

Copy link
Member

@joyeecheung joyeecheung Oct 27, 2017

Choose a reason for hiding this comment

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

Is it too much detail? AFAICT this is triggered by a failed pthread_create, may be we don't have to mention the watch dog thing, simply saying that node is unable to monitor SIGINT is enough? (ERR_REPL_SIGINTWATCH_FAILED)

Copy link
Member

Choose a reason for hiding this comment

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

EDIT: paste error, s/sigmask/pthread_create/

Copy link
Member

Choose a reason for hiding this comment

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

I agree, both the error code & the docs on it do have too much detail – ERR_CANNOT_WATCH_SIGINT or so should be fine.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

Move the throw out of c++ and into js using internal/errors
@jasnell jasnell force-pushed the migrate-util-sigintwatchdog branch from f99ee60 to 0d89d4d Compare October 30, 2017 00:39
@jasnell
Copy link
Member Author

jasnell commented Oct 30, 2017

ping @joyeecheung and @addaleax

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

mhdawson commented Nov 1, 2017

jasnell added a commit that referenced this pull request Nov 2, 2017
Move the throw out of c++ and into js using internal/errors

PR-URL: #16546
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@jasnell
Copy link
Member Author

jasnell commented Nov 2, 2017

Landed in 67c8511

@jasnell jasnell closed this Nov 2, 2017
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++. errors Issues and PRs related to JavaScript errors originated in Node.js core. repl Issues and PRs related to the REPL subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants