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.

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

domain: fix clearing domains stack #4659

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@misterdjules
Contributor

misterdjules commented Jan 13, 2016

Clear domains stack even if no domain error handler is set so that
code running in the process' uncaughtException handler, or any code that
may be executed when an error is thrown and not caught, doesn't run in
the context of the domain within which the error was thrown.

I set the semver-minor label conservatively because although in my opinion it fixes an inconsistency in domains' error handling, it's a potentially breaking behavior change.

I have a similar change for the v0.12.x branch, but we may not want to back port the change to the v0.12.x branch for the same reason. And finally, this test could also be back ported to the v0.10.x branch (the change itself doesn't need to, as v0.10.x versions are not affected by the problem this change fixes), with the same reserve.

Julien Gilli
domain: fix clearing domains stack
Clear domains stack __even if no domain error handler is set__ so that
code running in the process' uncaughtException handler, or any code that
may be executed when an error is thrown and not caught, doesn't run in
the context of the domain within which the error was thrown.
@misterdjules

This comment has been minimized.

Contributor

misterdjules commented Jan 13, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Jan 13, 2016

LGTM. Let's let this land in master and see if there are any issues with the potentially breaking change before we decide to pull it back to v4/v0.12/v0.10

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jan 13, 2016

LGTM.

Thanks @misterdjules for being willing to work on domains :-)

@misterdjules

This comment has been minimized.

Contributor

misterdjules commented Jan 13, 2016

@jasnell Sounds good, adding lts-watch-* labels.

@cjihrig My pleasure, and thank you for the review. I hope it helps!

All CI tests pass, except for regressions on Windows already triggered by previous changes, will land asap.

misterdjules pushed a commit that referenced this pull request Jan 13, 2016

Julien Gilli
domains: clear stack when no error handler
Clear domains stack __even if no domain error handler is set__ so that
code running in the process' uncaughtException handler, or any code that
may be executed when an error is thrown and not caught and that is not
the domain's error handler, doesn't run in the context of the domain
within which the error was thrown.

PR: #4659
PR-URL: #4659
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@misterdjules

This comment has been minimized.

Contributor

misterdjules commented Jan 13, 2016

Landed in 90204cc with a slightly reworded commit title/message. I assumed that these changes were small enough to not require another review cycle.

@evanlucas

This comment has been minimized.

Member

evanlucas commented Jan 18, 2016

@misterdjules you think this is good to land in v5.x as semver-minor?

@misterdjules

This comment has been minimized.

Contributor

misterdjules commented Jan 18, 2016

@evanlucas It's always tricky to make such a decision for bug fixes that change behavior. On one hand, this is a bug fix, so technically it could even be a semver-patch. On the other hand we've seen time and time again the most insignificant bug fixes break users' code.

This is an example where we can't simply rely on strict semver definitions to decide when to merge a change. For now, I wouldn't mind merging it only in the next major release (and thus make it a semver-major), but if in the meantime we get enough feedback from users that it's a bug that should be fixed in minor/LTS releases, then we should reevaluate.

@evanlucas

This comment has been minimized.

Member

evanlucas commented Jan 18, 2016

I do agree that with this being a behavior change, we should probably hold off and mark it as semver-major.

@jasnell

This comment has been minimized.

Member

jasnell commented Jan 18, 2016

Yep, let's keep the lts-watch tags in place for now but definitely hold off landing it there. /cc @nodejs/lts

@rvagg

This comment has been minimized.

Member

rvagg commented Jan 19, 2016

FWIW I think we can be slightly more flexible with semver deignations on v5, it's on the LTS branches where we have to be really strict and careful. I'd be fine with this landing in v5 but happy to go with the conservative option of semver-minor too.

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Jan 28, 2016

Adding the agenda tag so we can discuss this a bit more at length

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Mar 21, 2016

In the LTS meeting today this PR was discussed and it was decided that since this PR is only scheduled to land on v6 right now that we will remove the lts-watch labels and add dont-land labels for v4 and v5.

@misterdjules please feel free to chime in

@misterdjules

This comment has been minimized.

Contributor

misterdjules commented Mar 21, 2016

@thealphanerd Sounds good to me!

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

domains: clear stack when no error handler
Clear domains stack __even if no domain error handler is set__ so that
code running in the process' uncaughtException handler, or any code that
may be executed when an error is thrown and not caught and that is not
the domain's error handler, doesn't run in the context of the domain
within which the error was thrown.

PR: nodejs#4659
PR-URL: nodejs#4659
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@jasnell jasnell referenced this pull request Apr 19, 2016

Closed

What is new in v6? #6264

@jasnell jasnell removed the lts-agenda label Apr 25, 2016

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* OS X
  * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
    [#6402](#6402).
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* OS X
  * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
    [#6402](#6402).
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* OS X
  * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
    [#6402](#6402).
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

@misterdjules misterdjules deleted the misterdjules:clear-domains-stack-after-error-handling-done branch Jul 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment