lib,src: exit process on unhandled promise rejection cleanup #12010

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
@Fishrock123
Member

Fishrock123 commented Mar 23, 2017

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)

lib,src,process

Refs: #5292
Refs: nodejs/promises#26
Refs: #6355

This PR supersedes the previous PR from last year which was at #6375

This makes unhandled promise rejections exit the process though the usual exit handler if:

  • An unhandled, rejected promise is GC'd.
  • Unhandled promises are not handled by the time the process exits normally.

This is as per the current deprecation message.

Note: This is definitely not as clean as it could be. I adapted @addaleax's work directly as I was unable to make significant progress making it "nicer". I think ideally it would be closer to my initial implementation of track_promise.cc, but as linkedlist elements and stored somewhere in node.cc, or env. I think that would be more efficient and more clean. (Alas, C++ chops are not there. 馃槥)

Please take a look again @nodejs/ctc 馃槄

Edit: Big thanks to @matthewloring, @ofrobots, and @addaleax -- all of who helped me get this to even work again with the new V8 apis.

CI: https://ci.nodejs.org/job/node-test-pull-request/7006/

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Mar 24, 2017

Member

馃帀 awesome.

I'm still not sure we shouldn't warn after a sufficient amount of time anyway (like today's strategy with nextTick).

When exploring this there are several cases where a promise might not be GCd (for example if it is referenced in a closure) but is still unhandledRejection. For example IIRC:

function pingWebService() {
    var p = getWebServiceResult();
    async function persist() {
       let res = await p;
       await db.save(res);
    }
    async function dont() {
       console.log("HI");
    }
    return configuration.persist ? persist : dont;
}

IIRC, in the base above there is a reference to the promise in persist by callers of pingWebService - because persist and dont share the closure although dont doesn't have access to it. I'd expect to get a warning or to crash, but in this case I don't.

Member

benjamingr commented Mar 24, 2017

馃帀 awesome.

I'm still not sure we shouldn't warn after a sufficient amount of time anyway (like today's strategy with nextTick).

When exploring this there are several cases where a promise might not be GCd (for example if it is referenced in a closure) but is still unhandledRejection. For example IIRC:

function pingWebService() {
    var p = getWebServiceResult();
    async function persist() {
       let res = await p;
       await db.save(res);
    }
    async function dont() {
       console.log("HI");
    }
    return configuration.persist ? persist : dont;
}

IIRC, in the base above there is a reference to the promise in persist by callers of pingWebService - because persist and dont share the closure although dont doesn't have access to it. I'd expect to get a warning or to crash, but in this case I don't.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Mar 24, 2017

Member

Before we go into the nitty-gritty: you made it an always-on feature but tracking promises as weak references has clear performance implications. It should be an opt-in debug feature unless you can show the overhead is manageable.

Member

bnoordhuis commented Mar 24, 2017

Before we go into the nitty-gritty: you made it an always-on feature but tracking promises as weak references has clear performance implications. It should be an opt-in debug feature unless you can show the overhead is manageable.

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Mar 24, 2017

Member

When exploring this there are several cases where a promise might not be GCd (for example if it is referenced in a closure) but is still unhandledRejection.

@benjamingr We simply do not have enough information to exit the process due to the nature of promises. We could still warn, but that does seem some duplication of information.
Your process with still have a "fatal" exit if it were to shut down normally while a unhandled, rejected, but not GC'd promise still exists.


@bnoordhuis Correct. It has clear performance implications in a case where an error may be bringing down the process. If that is undesirable, a user can hook into the unhandledRejection event. Regardless of performance is a possible error exit case, this provides a good and consistent default for new users, with advanced users able to modify the behavior if they so wish.

Member

Fishrock123 commented Mar 24, 2017

When exploring this there are several cases where a promise might not be GCd (for example if it is referenced in a closure) but is still unhandledRejection.

@benjamingr We simply do not have enough information to exit the process due to the nature of promises. We could still warn, but that does seem some duplication of information.
Your process with still have a "fatal" exit if it were to shut down normally while a unhandled, rejected, but not GC'd promise still exists.


@bnoordhuis Correct. It has clear performance implications in a case where an error may be bringing down the process. If that is undesirable, a user can hook into the unhandledRejection event. Regardless of performance is a possible error exit case, this provides a good and consistent default for new users, with advanced users able to modify the behavior if they so wish.

var caught;
if (process.domain && process.domain._errorHandler)
caught = process.domain._errorHandler(er) || caught;
- if (!caught)
+ if (!caught && !fromPromise)

This comment has been minimized.

@addaleax

addaleax Mar 24, 2017

Member

What鈥檚 the reasoning for this? That it鈥檚 already made visible by process.on('unhandledRejection')?

@addaleax

addaleax Mar 24, 2017

Member

What鈥檚 the reasoning for this? That it鈥檚 already made visible by process.on('unhandledRejection')?

This comment has been minimized.

@Fishrock123

Fishrock123 Mar 24, 2017

Member

@addaleax Yes. That and to make sure we don't emit an event from GC that is able to be recovered from.

@Fishrock123

Fishrock123 Mar 24, 2017

Member

@addaleax Yes. That and to make sure we don't emit an event from GC that is able to be recovered from.

This comment has been minimized.

@addaleax

addaleax Mar 24, 2017

Member

@Fishrock123 oooh, that鈥檚 a good point 鈥 we should not be calling into the runtime from the GC at all

@addaleax

addaleax Mar 24, 2017

Member

@Fishrock123 oooh, that鈥檚 a good point 鈥 we should not be calling into the runtime from the GC at all

This comment has been minimized.

@Fishrock123

Fishrock123 Mar 24, 2017

Member

@addaleax Mmmm, not quite though. This still calls process.on('exit'), which seems to be ok and makes logical sense.

@Fishrock123

Fishrock123 Mar 24, 2017

Member

@addaleax Mmmm, not quite though. This still calls process.on('exit'), which seems to be ok and makes logical sense.

This comment has been minimized.

@addaleax

addaleax Mar 24, 2017

Member

@Fishrock123 We have definitely received bug reports of real-world V8 crashes because we tried to run JS code during GC, see b49b496 and its Fixes: tags

@addaleax

addaleax Mar 24, 2017

Member

@Fishrock123 We have definitely received bug reports of real-world V8 crashes because we tried to run JS code during GC, see b49b496 and its Fixes: tags

+
+ // If fn is empty we'll almost certainly have to panic anyways
+ return fn->Call(env->context(), Null(env->isolate()), 1,
+ &internal_props).ToLocalChecked();

This comment has been minimized.

@addaleax

addaleax Mar 24, 2017

Member

Can you align the parameters here?

@addaleax

addaleax Mar 24, 2017

Member

Can you align the parameters here?

// this will return true if the JS layer handled it, false otherwise
Local<Value> caught =
- fatal_exception_function->Call(process_object, 1, &error);
+ fatal_exception_function->Call(process_object, 2, argv);

This comment has been minimized.

@addaleax

addaleax Mar 24, 2017

Member

I think we鈥檝e been migrating towards using arraysize(argv) instead of hard-coding the argument count in Call()s

@addaleax

addaleax Mar 24, 2017

Member

I think we鈥檝e been migrating towards using arraysize(argv) instead of hard-coding the argument count in Call()s

+ FIXED_ONE_BYTE_STRING(env->isolate(), "Array")).As<Object>();
+ Local<Function> js_array_from_function = js_array_object->Get(
+ FIXED_ONE_BYTE_STRING(env->isolate(), "from")).As<Function>();
+ env->set_array_from(js_array_from_function);

This comment has been minimized.

@addaleax

addaleax Mar 24, 2017

Member

Is this used somewhere? It doesn鈥檛 look like that to me, but if yes: can you use the ->Get() overloads that take a context argument instead?

@addaleax

addaleax Mar 24, 2017

Member

Is this used somewhere? It doesn鈥檛 look like that to me, but if yes: can you use the ->Get() overloads that take a context argument instead?

This comment has been minimized.

@Fishrock123

Fishrock123 Mar 24, 2017

Member

Ah yeah, I think this is legacy code from when I used to walk the results array in JS.

@Fishrock123

Fishrock123 Mar 24, 2017

Member

Ah yeah, I think this is legacy code from when I used to walk the results array in JS.

+ gc();
+ gc();
+ gc();
+ /* eslint-enable no-undef */

This comment has been minimized.

@addaleax

addaleax Mar 24, 2017

Member

Do you need the eslint comments if you use global.gc() instead?

@addaleax

addaleax Mar 24, 2017

Member

Do you need the eslint comments if you use global.gc() instead?

+'use strict';
+const common = require('../common');
+
+// Flags: --expose-gc

This comment has been minimized.

@addaleax

addaleax Mar 24, 2017

Member

Can you move this comment to the top of the file?

@addaleax

addaleax Mar 24, 2017

Member

Can you move this comment to the top of the file?

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Mar 26, 2017

Member

@Fishrock123

We simply do not have enough information to exit the process due to the nature of promises. We could still warn, but that does seem some duplication of information.
Your process with still have a "fatal" exit if it were to shut down normally while a unhandled, rejected, but not GC'd promise still exists.

I agree that if it should crash it should crash. Let's observe a perhaps too common scenario. My main file is this though:

var p = readFileWithPromise("config.js");
// do stuff unrelated
p.then((config) => loadApp(config, "warn"));

In this scenario under GC-only unhandled rejection detection - the process never terminates and the error is swallowed. Under the current code - the user would at least get an informative error with a stack trace on why their program hangs.

We can make task based unhandled rejection tracking (a.k.a. what we currently have) less sensitive so it - for example - waits for 2 seconds before notifying the user of anything.

Even if we all agree that GC based is the only way to abort the process and is desirable - a warning for the case GC based detection won't work would be extremely invaluable when debugging. Note I am only suggesting a debug warning here.

Member

benjamingr commented Mar 26, 2017

@Fishrock123

We simply do not have enough information to exit the process due to the nature of promises. We could still warn, but that does seem some duplication of information.
Your process with still have a "fatal" exit if it were to shut down normally while a unhandled, rejected, but not GC'd promise still exists.

I agree that if it should crash it should crash. Let's observe a perhaps too common scenario. My main file is this though:

var p = readFileWithPromise("config.js");
// do stuff unrelated
p.then((config) => loadApp(config, "warn"));

In this scenario under GC-only unhandled rejection detection - the process never terminates and the error is swallowed. Under the current code - the user would at least get an informative error with a stack trace on why their program hangs.

We can make task based unhandled rejection tracking (a.k.a. what we currently have) less sensitive so it - for example - waits for 2 seconds before notifying the user of anything.

Even if we all agree that GC based is the only way to abort the process and is desirable - a warning for the case GC based detection won't work would be extremely invaluable when debugging. Note I am only suggesting a debug warning here.

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Mar 27, 2017

Contributor

We should crash by default in the absence of user-installed unhandledRejection handlers. This is a controversial stance (some notable promise-users disagree with me!), but I believe it's the best solution for Node's users in the long run. I will describe why I think this is a good behavior first, followed by why I think it's valid route for Node to take.

  • Crashing on unhandled rejection is more useful than logging: it prevents the program from taking further action (modulo nextTick and microtask calls 鈥 it's better behavior than we have now, not perfect).
  • It allows the program to more quickly & reliably enter a known-good state via restarting. Crashing on unhandled rejection aligns with Node's exception handling behavior, which is to terminate the program on unexpected behavior (notably: in that case we diverge from the browser, which continues the execution of the program!)
    • It is not perfect. We do not get usable core dumps from crashing on unhandled rejection. It is an incremental improvement in this case.
  • It is an acknowledgement that it is unsafe to publish modules that cause the execution of an externally-installed catch-all as a by-product of normal use (Would you use a module that relied on Node being able to survive an unhandled exception?)

Crashing on unhandled rejection is a valid path:

  • It does not break Promise.reject() 鈥 which will only trip unhandledRejection if a handler is not installed by next RunMicrotasks() call.
  • There's always a way to write a JavaScript program such that it doesn't trip the unhandled rejection handler. If you want to hang onto a rejection & handle it later, call myPromise.catch(() => {}). Later, when you want to handle the error, attaching another .catch will cause the error to propagate through that route.
  • As a migration path, we can add a --allow-unhandled-rejections flag to the Node release to retain old-mode behavior.

I don't think we should move forward with crash-on-GC as a default behavior. As a habitual promise user, it worries me:

  • The base promise implementation will hit this. Other implementations will not. In practice, in development, it's not uncommon to have a mix of promise implementations interoperating. Having one behavior for some of them and another behavior for others is confusing and bad. Adding crash-on-GC exacerbates this situation.
  • Crash-on-GC adds a perf hit for folks adopting safe behavior 鈥 if you've installed a crash-on-GC rejection handler, you still pay the tracking cost per promise. Were it an optional flag, I would not use it 鈥 with a number of different promise implementations in play, I would not find crash-on-GC useful to diagnose bugs with.
  • Crash-on-GC responds to memory pressure 鈥 which makes crashes subject to other conditions in the system.

As someone who develops & deploys production promise-based services, I would hesitate before migrating to a version of node that included crash-on-GC behavior by default. As a former Node CTC member, I'd advise caution before taking onboard the complexity of introducing it as a debugging tool.

Contributor

chrisdickinson commented Mar 27, 2017

We should crash by default in the absence of user-installed unhandledRejection handlers. This is a controversial stance (some notable promise-users disagree with me!), but I believe it's the best solution for Node's users in the long run. I will describe why I think this is a good behavior first, followed by why I think it's valid route for Node to take.

  • Crashing on unhandled rejection is more useful than logging: it prevents the program from taking further action (modulo nextTick and microtask calls 鈥 it's better behavior than we have now, not perfect).
  • It allows the program to more quickly & reliably enter a known-good state via restarting. Crashing on unhandled rejection aligns with Node's exception handling behavior, which is to terminate the program on unexpected behavior (notably: in that case we diverge from the browser, which continues the execution of the program!)
    • It is not perfect. We do not get usable core dumps from crashing on unhandled rejection. It is an incremental improvement in this case.
  • It is an acknowledgement that it is unsafe to publish modules that cause the execution of an externally-installed catch-all as a by-product of normal use (Would you use a module that relied on Node being able to survive an unhandled exception?)

Crashing on unhandled rejection is a valid path:

  • It does not break Promise.reject() 鈥 which will only trip unhandledRejection if a handler is not installed by next RunMicrotasks() call.
  • There's always a way to write a JavaScript program such that it doesn't trip the unhandled rejection handler. If you want to hang onto a rejection & handle it later, call myPromise.catch(() => {}). Later, when you want to handle the error, attaching another .catch will cause the error to propagate through that route.
  • As a migration path, we can add a --allow-unhandled-rejections flag to the Node release to retain old-mode behavior.

I don't think we should move forward with crash-on-GC as a default behavior. As a habitual promise user, it worries me:

  • The base promise implementation will hit this. Other implementations will not. In practice, in development, it's not uncommon to have a mix of promise implementations interoperating. Having one behavior for some of them and another behavior for others is confusing and bad. Adding crash-on-GC exacerbates this situation.
  • Crash-on-GC adds a perf hit for folks adopting safe behavior 鈥 if you've installed a crash-on-GC rejection handler, you still pay the tracking cost per promise. Were it an optional flag, I would not use it 鈥 with a number of different promise implementations in play, I would not find crash-on-GC useful to diagnose bugs with.
  • Crash-on-GC responds to memory pressure 鈥 which makes crashes subject to other conditions in the system.

As someone who develops & deploys production promise-based services, I would hesitate before migrating to a version of node that included crash-on-GC behavior by default. As a former Node CTC member, I'd advise caution before taking onboard the complexity of introducing it as a debugging tool.

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Mar 27, 2017

Contributor

We simply do not have enough information to exit the process due to the nature of promises.

I suspect this is probably the crux of disagreement: that we should preserve Node's ability to run the following code such that it outputs "hello world", hitting any user-installed unhandledRejection handlers:

const rejected = Promise.reject(new Error('unhandled, for now'));

setTimeout(() => {
  rejected.catch(() => console.log("hello world"));
}, 1000);

In a world where we crash on unhandled rejection, there are two options to maintain the above behavior:

// option 1:
const rejected = Promise.reject(new Error('unhandled, for now'));
rejected.catch(() => {}) // "we know we will handle this later"

setTimeout(() => {
  rejected.catch(() => console.log("hello world"));
}, 1000);

// option 2:
const rejected = Promise.reject(new Error('unhandled, for now'));
process.on('unhandledRejection', () => {}) // "it's okay to not handle rejections between ticks"

setTimeout(() => {
  rejected.catch(() => console.log("hello world"));
}, 1000);
Contributor

chrisdickinson commented Mar 27, 2017

We simply do not have enough information to exit the process due to the nature of promises.

I suspect this is probably the crux of disagreement: that we should preserve Node's ability to run the following code such that it outputs "hello world", hitting any user-installed unhandledRejection handlers:

const rejected = Promise.reject(new Error('unhandled, for now'));

setTimeout(() => {
  rejected.catch(() => console.log("hello world"));
}, 1000);

In a world where we crash on unhandled rejection, there are two options to maintain the above behavior:

// option 1:
const rejected = Promise.reject(new Error('unhandled, for now'));
rejected.catch(() => {}) // "we know we will handle this later"

setTimeout(() => {
  rejected.catch(() => console.log("hello world"));
}, 1000);

// option 2:
const rejected = Promise.reject(new Error('unhandled, for now'));
process.on('unhandledRejection', () => {}) // "it's okay to not handle rejections between ticks"

setTimeout(() => {
  rejected.catch(() => console.log("hello world"));
}, 1000);
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 9, 2017

Member

@Fishrock123 This would need a rebase

Member

addaleax commented Apr 9, 2017

@Fishrock123 This would need a rebase

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Apr 14, 2017

Member

@Fishrock123 how do we progress from here?

Member

benjamingr commented Apr 14, 2017

@Fishrock123 how do we progress from here?

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Apr 17, 2017

Member

I don't know. I am largely unwilling to deal with the potential "promises people" (whatever the hell that means) / TC39 backlash against erroring in next tick (when the event happens), though in reality it makes far more sense as @chrisdickinson says.

Member

Fishrock123 commented Apr 17, 2017

I don't know. I am largely unwilling to deal with the potential "promises people" (whatever the hell that means) / TC39 backlash against erroring in next tick (when the event happens), though in reality it makes far more sense as @chrisdickinson says.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Apr 17, 2017

Member

As a representative of "the promises" people - why? Erroring in the next tick is fine.

Member

benjamingr commented Apr 17, 2017

As a representative of "the promises" people - why? Erroring in the next tick is fine.

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Apr 17, 2017

Contributor

I don't know that I have a ton to contribute, but I'll say the cases that concern me around this are mostly async function cases where there isn't such obvious devision between "handled" vs "unhandled" as there is in #12010 (comment).

I could imagine writing code like this:

async function doOperation() {
  const resultOne = randomFirstOpMayReject(); 

  const resultTwo = randomSecondOpMayReject();

  try {
    await resultOne;
  } catch (e) {
    // handle error
  }

  try {
    await resultTwo;
  } catch (e) {
    // handle error
  }
}

if I want both operations to run in parallel, but I expect to be able to handle the errors.

If I understand right, if the first operation takes a while to complete, and the second throws, it'll be an unhandled rejection even though the code is written to handle both errors.

Throwing on .nextTick is a good compromise if you're manually chaining the promises, but it seems unlikely that people writing async functions would stop to think about these cases.

(Fishrock123: edited to highlight code)

Contributor

loganfsmyth commented Apr 17, 2017

I don't know that I have a ton to contribute, but I'll say the cases that concern me around this are mostly async function cases where there isn't such obvious devision between "handled" vs "unhandled" as there is in #12010 (comment).

I could imagine writing code like this:

async function doOperation() {
  const resultOne = randomFirstOpMayReject(); 

  const resultTwo = randomSecondOpMayReject();

  try {
    await resultOne;
  } catch (e) {
    // handle error
  }

  try {
    await resultTwo;
  } catch (e) {
    // handle error
  }
}

if I want both operations to run in parallel, but I expect to be able to handle the errors.

If I understand right, if the first operation takes a while to complete, and the second throws, it'll be an unhandled rejection even though the code is written to handle both errors.

Throwing on .nextTick is a good compromise if you're manually chaining the promises, but it seems unlikely that people writing async functions would stop to think about these cases.

(Fishrock123: edited to highlight code)

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Apr 17, 2017

Member

@loganfsmyth

I could imagine writing code like this:

Note that this PR introduces GC based promise unhandled rejection detection - and would not emit an unhnaldedRejection for that particular code in any case.

Member

benjamingr commented Apr 17, 2017

@loganfsmyth

I could imagine writing code like this:

Note that this PR introduces GC based promise unhandled rejection detection - and would not emit an unhnaldedRejection for that particular code in any case.

@loganfsmyth

This comment has been minimized.

Show comment
Hide comment
@loganfsmyth

loganfsmyth Apr 17, 2017

Contributor

Yup! I'm fully in support of this PR, just trying to provide an outside viewpoint, I'll leave you all to it :)

Contributor

loganfsmyth commented Apr 17, 2017

Yup! I'm fully in support of this PR, just trying to provide an outside viewpoint, I'll leave you all to it :)

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 May 1, 2017

Member

I think can largely fix the direct technical issues with this PR.
Upon further thinking:

  • GC hooks are available so we should be able to register one to get notified when GC ends so we can emit 'exit'.
  • The {un}trackPromise hooks should be fine to make publicly available for promise library implementors.
Member

Fishrock123 commented May 1, 2017

I think can largely fix the direct technical issues with this PR.
Upon further thinking:

  • GC hooks are available so we should be able to register one to get notified when GC ends so we can emit 'exit'.
  • The {un}trackPromise hooks should be fine to make publicly available for promise library implementors.
@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson May 2, 2017

Contributor

@loganfsmyth: For posterity, you would solve this problem by doing the following:

async function doOperation() {
  const resultOne = randomFirstOpMayReject(); 

  const resultTwo = randomSecondOpMayReject();
  resultTwo.catch(() => {}); // "I intend to catch any exception from resultTwo later."
  try {
    await resultOne;
  } catch (e) {
    // handle error
  }

  try {
    await resultTwo;
  } catch (e) {
    // handle error
  }
}
Contributor

chrisdickinson commented May 2, 2017

@loganfsmyth: For posterity, you would solve this problem by doing the following:

async function doOperation() {
  const resultOne = randomFirstOpMayReject(); 

  const resultTwo = randomSecondOpMayReject();
  resultTwo.catch(() => {}); // "I intend to catch any exception from resultTwo later."
  try {
    await resultOne;
  } catch (e) {
    // handle error
  }

  try {
    await resultTwo;
  } catch (e) {
    // handle error
  }
}
@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Jul 21, 2017

What's the status of this? Is there a plan to have it as some point?

At least introducing unhandled rejections boilerplate to every script to ensure proper error exposure is quite annoying, it's not how Node.js worked before promises.

medikoo commented Jul 21, 2017

What's the status of this? Is there a plan to have it as some point?

At least introducing unhandled rejections boilerplate to every script to ensure proper error exposure is quite annoying, it's not how Node.js worked before promises.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jul 21, 2017

@medikoo it's precisely how node.js worked before promises; errors thrown inside try/catches weren't surfaced anywhere - just like errors thrown inside promises.

ljharb commented Jul 21, 2017

@medikoo it's precisely how node.js worked before promises; errors thrown inside try/catches weren't surfaced anywhere - just like errors thrown inside promises.

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Jul 21, 2017

@medikoo it's precisely how node.js worked before promises; errors thrown inside try/catches weren't surfaced anywhere - just like errors thrown inside promises

@ljharb nobody constructed async flows with try/catch'es :)

medikoo commented Jul 21, 2017

@medikoo it's precisely how node.js worked before promises; errors thrown inside try/catches weren't surfaced anywhere - just like errors thrown inside promises

@ljharb nobody constructed async flows with try/catch'es :)

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Jul 21, 2017

@ljharb Node.js since very early days, exposes really great pattern of throwing errors to which you did not subscribe (eventEmitter that's shared by most of interfaces), and exactly same thing should happen with promises. It's a bummer it's not the case

medikoo commented Jul 21, 2017

@ljharb Node.js since very early days, exposes really great pattern of throwing errors to which you did not subscribe (eventEmitter that's shared by most of interfaces), and exactly same thing should happen with promises. It's a bummer it's not the case

@medikoo medikoo referenced this pull request Jul 21, 2017

Closed

fs: throw on invalid callbacks for async functions #12562

3 of 3 tasks complete

@medikoo medikoo referenced this pull request in serverless/serverless Aug 22, 2017

Merged

Fix tests setup - ensure errors are exposed #4139

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Aug 25, 2017

@Fishrock123 @benjamingr what's the status of it?

Many popular projects suffer cause of unhandled errors not being thrown (exposed) e.g. see:

serverless/serverless#4139
mochajs/mocha#2640

medikoo commented Aug 25, 2017

@Fishrock123 @benjamingr what's the status of it?

Many popular projects suffer cause of unhandled errors not being thrown (exposed) e.g. see:

serverless/serverless#4139
mochajs/mocha#2640

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Aug 25, 2017

Member

idk performance issues or something

i, too, wish this was still an idea that might get somewhere but I simply do not have the energy to get it there

Member

Fishrock123 commented Aug 25, 2017

idk performance issues or something

i, too, wish this was still an idea that might get somewhere but I simply do not have the energy to get it there

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Aug 25, 2017

Member

I just had a look at this and for me it seems pretty solid and important. I am not sure if I understand the comments about the performance penalty correct: as far as I see it the code will only have a negative impact on unhandled rejections. I think these are fairly rare in average code and correctness is more important then performance.

But to be sure about the implications: @Fishrock123 would you be so kind and rebase and add a benchmark to see how bad it really is? I think the average use case will not be penalized.

Besides that other @nodejs/collaborators might weight in as well.

Member

BridgeAR commented Aug 25, 2017

I just had a look at this and for me it seems pretty solid and important. I am not sure if I understand the comments about the performance penalty correct: as far as I see it the code will only have a negative impact on unhandled rejections. I think these are fairly rare in average code and correctness is more important then performance.

But to be sure about the implications: @Fishrock123 would you be so kind and rebase and add a benchmark to see how bad it really is? I think the average use case will not be penalized.

Besides that other @nodejs/collaborators might weight in as well.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 25, 2017

Member

But to be sure about the implications: @Fishrock123 would you be so kind and rebase and add a benchmark to see how bad it really is?

...or, if you're comfortable with it, maybe give @BridgeAR permission to rebase and push directly to this branch and try to get this across the finish line? (I'm making an assumption that @BridgeAR is up for it. If I'm wrong, sorry about that.)

Member

Trott commented Aug 25, 2017

But to be sure about the implications: @Fishrock123 would you be so kind and rebase and add a benchmark to see how bad it really is?

...or, if you're comfortable with it, maybe give @BridgeAR permission to rebase and push directly to this branch and try to get this across the finish line? (I'm making an assumption that @BridgeAR is up for it. If I'm wrong, sorry about that.)

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 25, 2017

@BridgeAR they're not necessarily rare; and the language is explicitly designed to allow for the program to continue when there are unhandled rejections - both because they might be handled later, and because the language does not require an explicit .catch() when you intend the error to be swallowed.

ljharb commented Aug 25, 2017

@BridgeAR they're not necessarily rare; and the language is explicitly designed to allow for the program to continue when there are unhandled rejections - both because they might be handled later, and because the language does not require an explicit .catch() when you intend the error to be swallowed.

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Aug 25, 2017

Member

@ljharb this is only about failing in case of a unhandled rejection that is garbage collected so the program would continue in case of unhandled rejetions that are handled later on (as the tests also show). As far as I see it this was also discussed a lot before and those discussions lead to this PR in the first place (since this is a relatively safe way of dealing with this edge case). Those which are handled later will probably have a minor performance penalty but otherwise they should not be affected. That the language does not explicitly require a catch handler is fine but this is only about how Node.js deals with these cases and it was already decided to terminate Node.js in case a real unhandled rejection occurred. Besides the fact that even with this code anyone can actually decide to further ignore real unhandled rejections by simply using process.on("unhandledRejection", () => {}).

To conclude - I think this PR improves the situation for almost anyone and especially new comers and I think it would be great to get this into core.

Member

BridgeAR commented Aug 25, 2017

@ljharb this is only about failing in case of a unhandled rejection that is garbage collected so the program would continue in case of unhandled rejetions that are handled later on (as the tests also show). As far as I see it this was also discussed a lot before and those discussions lead to this PR in the first place (since this is a relatively safe way of dealing with this edge case). Those which are handled later will probably have a minor performance penalty but otherwise they should not be affected. That the language does not explicitly require a catch handler is fine but this is only about how Node.js deals with these cases and it was already decided to terminate Node.js in case a real unhandled rejection occurred. Besides the fact that even with this code anyone can actually decide to further ignore real unhandled rejections by simply using process.on("unhandledRejection", () => {}).

To conclude - I think this PR improves the situation for almost anyone and especially new comers and I think it would be great to get this into core.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 25, 2017

Yes, I totally agree that only failing on unhandled GC is acceptable; I was just pointing out that it's not necessarily rare, and this change is probably going to cause a lot of people to add a .catch() to avoid the crash (which was an acceptable part of the lengthy discussion you referred to)

ljharb commented Aug 25, 2017

Yes, I totally agree that only failing on unhandled GC is acceptable; I was just pointing out that it's not necessarily rare, and this change is probably going to cause a lot of people to add a .catch() to avoid the crash (which was an acceptable part of the lengthy discussion you referred to)

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 25, 2017

Member

I' mean currently in Holiday but if you would like I'm game on discussing the implications in a call. In addition there is ongoing work by @uzon at nodejs/post-mortem about promises.

In a gist the problem is that either with GC and "after next tick" there are false positives or false negatives and the problem of deciding if a rejection is ever handled is easily reducible to the halting problem.

I think the future is with schedulers or contexts where you can declare a scope (like an HTTP request) where rejections can be handled - we might be able to do that - and that might burden frameworks a little but users will benefit immensely.

Member

benjamingr commented Aug 25, 2017

I' mean currently in Holiday but if you would like I'm game on discussing the implications in a call. In addition there is ongoing work by @uzon at nodejs/post-mortem about promises.

In a gist the problem is that either with GC and "after next tick" there are false positives or false negatives and the problem of deciding if a rejection is ever handled is easily reducible to the halting problem.

I think the future is with schedulers or contexts where you can declare a scope (like an HTTP request) where rejections can be handled - we might be able to do that - and that might burden frameworks a little but users will benefit immensely.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 25, 2017

Member

To clarify - GC based detection has false negatives that current unhandled rejection detection doesn't and they are significant. For example imagine not detecting a bug in an exception because of a memory leak - sounds terrible (not to mention just regular global sand anything with long lifetime)

Member

benjamingr commented Aug 25, 2017

To clarify - GC based detection has false negatives that current unhandled rejection detection doesn't and they are significant. For example imagine not detecting a bug in an exception because of a memory leak - sounds terrible (not to mention just regular global sand anything with long lifetime)

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Aug 25, 2017

Member

@benjamingr I think it would be fine to add the warning back in to be on the safe side and just change the message a bit and remove the deprecation warning. That way it would be a bit redundant in case of a exit but we do not loose any information either. I guess that would be your ideal solution, right?

Member

BridgeAR commented Aug 25, 2017

@benjamingr I think it would be fine to add the warning back in to be on the safe side and just change the message a bit and remove the deprecation warning. That way it would be a bit redundant in case of a exit but we do not loose any information either. I guess that would be your ideal solution, right?

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 25, 2017

Member

It wouldn't be ideal but I'm +1 on it because I think it would help a lot of people. It壮s better than what we have right now anyway.

Member

benjamingr commented Aug 25, 2017

It wouldn't be ideal but I'm +1 on it because I think it would help a lot of people. It壮s better than what we have right now anyway.

@BridgeAR BridgeAR referenced this pull request Sep 1, 2017

Closed

lib/src: exit on gc unhandled promise #15126

2 of 4 tasks complete
@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 20, 2017

Member

@Fishrock123 I am closing this for now to reduce the amount of open PRs. I am going to follow up on this in my PR #15126 very soon. If you would like to do that instead, I would very much appreciate that as well!

Member

BridgeAR commented Sep 20, 2017

@Fishrock123 I am closing this for now to reduce the amount of open PRs. I am going to follow up on this in my PR #15126 very soon. If you would like to do that instead, I would very much appreciate that as well!

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