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.

Sign up for GitHub

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

Jump to bottom

domains: fix handling of uncaught exceptions #3884

Closed
wants to merge 2 commits into from
Closed

domains: fix handling of uncaught exceptions #3884

wants to merge 2 commits into from

Conversation

misterdjules
Copy link

Fix node exiting due to an exception being thrown rather than emitting
an 'uncaughtException' event on the process object when:

  1. no error handler is set on the domain within which an error is thrown
  2. an 'uncaughtException' event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and --abort-on-uncaught-exception is used.

Fixes #3607 and #3653.

@misterdjules misterdjules added domain Issues and PRs related to the domain subsystem. lts-watch-v4.x labels Nov 17, 2015
@misterdjules
Copy link
Author

This PR corresponds to #3654 minus the behavior change mentioned in #3654 (comment).

/cc @nodejs/lts @nodejs/ctc @nodejs/collaborators

@jasnell
Copy link
Member

jasnell commented Nov 17, 2015

LGTM. Would appreciate a few more eyes on it tho @nodejs/lts

@misterdjules misterdjules mentioned this pull request Nov 17, 2015
Closed
@misterdjules misterdjules force-pushed the fix-uncaught-exceptions-domains-v4.x branch from 9d31c87 to 608b8dd Compare November 23, 2015 23:35
@misterdjules
Copy link
Author

Updated this PR with code review comments from #3654 that apply to the v4.x branch.

@misterdjules misterdjules force-pushed the fix-uncaught-exceptions-domains-v4.x branch from 608b8dd to 0b01280 Compare November 24, 2015 18:55
@misterdjules
Copy link
Author

Updated again from code review comments in #3654 that apply to the v4.x branch.

@misterdjules
Copy link
Author

CI tests running.

Julien Gilli added 2 commits December 11, 2015 15:01
domains: fix handling of uncaught exceptions
20a6656 
Fix node exiting due to an exception being thrown rather than emitting
an `'uncaughtException'` event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an `'uncaughtException'` event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and `--abort-on-uncaught-exception` is used.

Fixes nodejs#3607 and nodejs#3653.
Changes according to latest code review
6e3901b
@misterdjules misterdjules force-pushed the fix-uncaught-exceptions-domains-v4.x branch from d7aabca to 6e3901b Compare December 11, 2015 23:02
@misterdjules
Copy link
Author

Back ported changes according to the latest round of reviews in #3654, and started new CI tests.

/cc @nodejs/ctc @nodejs/lts

@misterdjules
Copy link
Author

For reviewers: this PR is basically identical to #3654, except for a behavior change that landed in master, but not in this PR targeted to v4.x.

As a result, the test in master that checks that having an error handler in any enclosing domain does not make the process abort when using --abort-on-uncaught-exception has been made into the opposite test that checks that the process actually aborts.

@misterdjules
Copy link
Author

All tests passed, except for existing regressions on Windows and Linux.

@jasnell
Copy link
Member

jasnell commented Dec 12, 2015

LGTM

misterdjules pushed a commit that referenced this pull request Dec 16, 2015
domains: fix handling of uncaught exceptions
25cded4 
Fix node exiting due to an exception being thrown rather than emitting
an `'uncaughtException'` event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an `'uncaughtException'` event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and `--abort-on-uncaught-exception` is used.

Fixes #3607 and #3653.

PR: #3884
PR-URL: #3884
Reviewed-By: James M Snell <jasnell@gmail.com>
@misterdjules
Copy link
Author

Landed in 25cded4.

@misterdjules
Copy link
Author

@nodejs/lts Just to make sure I'm using labels properly, is the lts-landed-on-v4.x label used to mean that a PR landed in v4.x-staging, or that it landed in v4.x?

@jasnell
Copy link
Member

jasnell commented Dec 16, 2015

Use lts-watch-4.x to indicate that it needs to land in v4.x-staging
Use land-on-v4.x to indicate that it's in v4.x-staging but still needs to land in v4.x
The lts-landed-on-v4.x would only apply after it's landed on v4.x, but that label might get dropped, it's not overly useful

@misterdjules
Copy link
Author

@jasnell Thank you for the clarification. It might be worth it to clarify that in the LTS section of the collaborators guide. I'll see if I can find some time to send a PR and maybe make that aspect clearer.

@jasnell jasnell mentioned this pull request Dec 17, 2015
Closed
misterdjules pushed a commit that referenced this pull request Dec 17, 2015
@jasnell
domains: fix handling of uncaught exceptions
34e64e5 
Fix node exiting due to an exception being thrown rather than emitting
an `'uncaughtException'` event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an `'uncaughtException'` event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and `--abort-on-uncaught-exception` is used.

Fixes #3607 and #3653.

PR: #3884
PR-URL: #3884
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell added a commit that referenced this pull request Dec 17, 2015
@jasnell
2015-12-17, Version 4.2.4 "Argon" (LTS) Release
ca439ce 
Maintenance Update

Notable changes

* Roughly 78% of the commits are documentation and test
  improvements
* domains:
  - Fix handling of uncaught exceptions (Julien Gilli)
    [#3884](#3884)
* deps:
  - Upgrade to npm 2.14.12 (Kat Marchán)
    [#4110](#4110)
  - Backport 819b40a from V8 upstream (Michaël Zasso)
    [#3938](#3938)
  - Updated node LICENSE file with new npm license (Kat Marchán)
    [#4110](#4110)
@jasnell jasnell mentioned this pull request Dec 17, 2015
Closed
jasnell added a commit that referenced this pull request Dec 21, 2015
@jasnell
2015-12-17, Version 4.2.4 "Argon" (LTS) Release
6fe5a18 
Maintenance Update

Notable changes

* Roughly 78% of the commits are documentation and test
  improvements
* domains:
  - Fix handling of uncaught exceptions (Julien Gilli)
    [#3884](#3884)
* deps:
  - Upgrade to npm 2.14.12 (Kat Marchán)
    [#4110](#4110)
  - Backport 819b40a from V8 upstream (Michaël Zasso)
    [#3938](#3938)
  - Updated node LICENSE file with new npm license (Kat Marchán)
    [#4110](#4110)
misterdjules pushed a commit that referenced this pull request Dec 23, 2015
@jasnell
domains: fix handling of uncaught exceptions
88e8d93 
Fix node exiting due to an exception being thrown rather than emitting
an `'uncaughtException'` event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an `'uncaughtException'` event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and `--abort-on-uncaught-exception` is used.

Fixes #3607 and #3653.

PR: #3884
PR-URL: #3884
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell added a commit that referenced this pull request Dec 23, 2015
@jasnell
2015-12-23, Version 4.2.4 "Argon" (LTS) Release
13151aa 
Maintenance Update

Notable changes

* Roughly 78% of the commits are documentation and test
  improvements
* domains:
  - Fix handling of uncaught exceptions (Julien Gilli)
    [#3884](#3884)
* deps:
  - Upgrade to npm 2.14.12 (Kat Marchán)
    [#4110](#4110)
  - Backport 819b40a from V8 upstream (Michaël Zasso)
    [#3938](#3938)
  - Updated node LICENSE file with new npm license (Kat Marchán)
    [#4110](#4110)
jasnell added a commit that referenced this pull request Dec 23, 2015
@jasnell
2015-12-23, Version 4.2.4 "Argon" (LTS) Release
7fac47c 
Maintenance Update

Notable changes

* Roughly 78% of the commits are documentation and test
  improvements
* domains:
  - Fix handling of uncaught exceptions (Julien Gilli)
    [#3884](#3884)
* deps:
  - Upgrade to npm 2.14.12 (Kat Marchán)
    [#4110](#4110)
  - Backport 819b40a from V8 upstream (Michaël Zasso)
    [#3938](#3938)
  - Updated node LICENSE file with new npm license (Kat Marchán)
    [#4110](#4110)
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@jasnell
2015-12-23, Version 4.2.4 "Argon" (LTS) Release
b6bec59 
Maintenance Update

Notable changes

* Roughly 78% of the commits are documentation and test
  improvements
* domains:
  - Fix handling of uncaught exceptions (Julien Gilli)
    [nodejs#3884](nodejs#3884)
* deps:
  - Upgrade to npm 2.14.12 (Kat Marchán)
    [nodejs#4110](nodejs#4110)
  - Backport 819b40a from V8 upstream (Michaël Zasso)
    [nodejs#3938](nodejs#3938)
  - Updated node LICENSE file with new npm license (Kat Marchán)
    [nodejs#4110](nodejs#4110)
@misterdjules misterdjules deleted the fix-uncaught-exceptions-domains-v4.x branch July 24, 2017 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
domain Issues and PRs related to the domain subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@misterdjules @jasnell @MylesBorins