FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

domains: fix handling of uncaught exceptions by misterdjules · Pull Request #3884 · nodejs/node · GitHub

/ node Public

domains: fix handling of uncaught exceptions - #3884

Closed
misterdjules wants to merge 2 commits into
nodejs:v4.x-stagingfrom
misterdjules:fix-uncaught-exceptions-domains-v4.x
ConversationCommits (2)ChecksFiles changed
Closed

domains: fix handling of uncaught exceptions#3884
misterdjules wants to merge 2 commits into
nodejs:v4.x-stagingfrom
misterdjules:fix-uncaught-exceptions-domains-v4.x

Conversation

misterdjules commented Nov 17, 2015

Copy link
Copy Markdown

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 added domain Issues and PRs related to the domain subsystem. lts-watch-v4.x labels Nov 17, 2015

misterdjules commented Nov 17, 2015

Copy link
Copy Markdown
Author

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

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

jasnell commented Nov 17, 2015

Copy link
Copy Markdown
Member

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

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

misterdjules commented Nov 23, 2015

Copy link
Copy Markdown
Author

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

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

misterdjules commented Nov 24, 2015

Copy link
Copy Markdown
Author

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

misterdjules commented Nov 27, 2015

Copy link
Copy Markdown
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 force-pushed the fix-uncaught-exceptions-domains-v4.x branch from d7aabca to 6e3901b Compare December 11, 2015 23:02

misterdjules commented Dec 11, 2015

Copy link
Copy Markdown
Author

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

/cc @nodejs/ctc @nodejs/lts

misterdjules commented Dec 11, 2015

Copy link
Copy Markdown
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 commented Dec 11, 2015

Copy link
Copy Markdown
Author

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

jasnell commented Dec 12, 2015

Copy link
Copy Markdown
Member

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 commented Dec 16, 2015

Copy link
Copy Markdown
Author

Landed in 25cded4.

misterdjules commented Dec 16, 2015

Copy link
Copy Markdown
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 commented Dec 16, 2015

Copy link
Copy Markdown
Member

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 commented Dec 17, 2015

Copy link
Copy Markdown
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 mentioned this pull request Dec 17, 2015
Closed
misterdjules pushed a commit that referenced this pull request Dec 17, 2015
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
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 mentioned this pull request Dec 17, 2015
Closed
jasnell added a commit that referenced this pull request Dec 21, 2015
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
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
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
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
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 deleted the fix-uncaught-exceptions-domains-v4.x branch July 24, 2017 17:35
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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.

3 participants

Back | FazBrowse Home | New Git URL