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.

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

async_hooks: async hook stack assertion shadows "Maximum call stack size exceeded" #15448

Closed
ilyaigpetrov opened this issue Sep 17, 2017 · 8 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. confirmed-bug Issues with confirmed bugs.

Comments

@ilyaigpetrov
Copy link

ilyaigpetrov commented Sep 17, 2017

  • Node Version: v8.2.1
  • Platform: Linux 4.10.0-33-generic #37~16.04.1-Ubuntu SMP Fri Aug 11 14:07:24 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
  • node-fetch@1.7.3

My product is not affected by this bug, but I report it anyway because it's still a bug and may affect someone else.

'use strict';

const fetch = require('node-fetch');

// Function name `fetch` clashes with required `fetch`!
const FooBar = async function fetch(url, encoding) {

  const res = fetch(url); // Programmer forgot `await` here!
  const text = await res.text();

};

(async function () {
  return await FooBar('https://github.com');
})();
$ node foobar.js 
Error: async hook stack has become corrupted (actual: 0, expected: 1)
 1: node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) [node]
 2: node::Start(int, char**) [node]
 3: __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
 4: 0x8ec861 [node]

Expected

User-friendly error message is expected, not this stack error.

@mscdex mscdex added the async_hooks Issues and PRs related to the async hooks subsystem. label Sep 17, 2017
@addaleax addaleax added the confirmed-bug Issues with confirmed bugs. label Sep 17, 2017
@addaleax
Copy link
Member

/cc @nodejs/async_hooks

I can reproduce this with master

@refack
Copy link
Contributor

refack commented Sep 17, 2017

I think is mostly a stack overflow issue; fetch is being reassigned on L6, then called recursivly on L8:

6: const FooBar = async function fetch(url, encoding) {
7:
8:   const res = fetch(url); // Programmer forgot `await` here!

But I agree the error should be RangeError: Maximum call stack size exceeded and not the C++ assertion error.

@addaleax
Copy link
Member

addaleax commented Sep 17, 2017

Standalone reproduction:

async function fn() {
  fn();
  throw new Error();
}

(async function() { await fn(); })()

Edit: Without the throw, this fails silently, so there’s definitely more going on here.

@refack refack changed the title Error: async hook stack has become corrupted (actual: 0, expected: 1) async_hooks: async hook stack assertion shadows "Maximum call stack size exceeded" Sep 17, 2017
@AndreasMadsen
Copy link
Member

#15454 skips the sanity check when async_hooks is disabled. It is not exactly a fix for this, but it is related.

@trevnorris
Copy link
Contributor

Thank you. Looking into it.

@trevnorris
Copy link
Contributor

trevnorris commented Sep 22, 2017

Thanks for reporting this. Quick update, this has to do w/ Promises and async_hooks not being enabled:

(node::node_async_ids) ids = (async_id = 2.8803866749967284E-310, trigger_id = 8.8796076974385372E-311)
(node::Environment *) env = 0x00007fffffffc608                                                         

whereas:

require('async_hooks').createHook({ init() { } }).enable();

async function fn() {
  fn();
  throw new Error();
}

(async function() { await fn(); })()

gives:

/tmp/15448.js:3
async function fn() {
                 ^

RangeError: Maximum call stack size exceeded
    at <anonymous>
    at fn (/tmp/15448.js:3:18)
    at fn (/tmp/15448.js:4:3)
    at fn (/tmp/15448.js:4:3)
    at fn (/tmp/15448.js:4:3)
    at fn (/tmp/15448.js:4:3)
    at fn (/tmp/15448.js:4:3)
    at fn (/tmp/15448.js:4:3)
    at fn (/tmp/15448.js:4:3)
    at fn (/tmp/15448.js:4:3)

@trevnorris
Copy link
Contributor

trevnorris commented Sep 22, 2017

There also seems to be a discrepancy depending on when this runs:

process.nextTick(() => {
  async function fn() {
    fn();
    throw new Error();
  }
  (async function() { await fn(); })()
});

produces

(node:PID) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: ID): Error

over 4,000 times, and has an exit code of 0. Also investigating that.

EDIT: nix that. Simply changing the script to

async function fn() {
  fn();
  throw new Error();
}
(async function() { await fn(); })()
process._rawDebug('------ AFTER -----');

outputs the same as above.

@trevnorris
Copy link
Contributor

Have a fix at #15553. Thanks for the bug report.

trevnorris added a commit to trevnorris/node that referenced this issue Sep 27, 2017
If bootstrap throws and if ids are added to the async id stack and if
the exception wasn't handled by the fatal exception handler then the
AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack
check to fail. Causing the application to crash. So clear the async id
stack manually.

This is only possible if the user: 1) manually calls MakeCallback() or
2) uses async await in the top level. Which will cause _tickCallback()
to fire before bootstrap finishes executing.

The following example shows how the application can fail due to
exceeding the maximum call stack while using async await:

    async function fn() {
      fn();
      throw new Error();
    }
    (async function() { await fn(); })();

If this occurs during bootstrap then the application will pring the
following warning a number of times then exit with a status of 0:

    (node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error

Here's the same recursive call done after enabling a new AsyncHook() the
following will print instead of the above warning and exit with a
non-zero code (currently it's 7 because of how node::FatalException
assigns error codes based on where the failure happened):

    script.js:25
    async function fn() {
                     ^
    RangeError: Maximum call stack size exceeded
        at <anonymous>
        at fn (script.js:25:18)
        at fn (script.js:26:3)
        ....

This has to do with how Promises lazily enable PromiseHook if an
AsyncHook() is enabled. Whether these need to be made uniform is outside
the scope of this commit

Fixes: nodejs#15448
PR-URL: nodejs#15553
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this issue Sep 28, 2017
If bootstrap throws and if ids are added to the async id stack and if
the exception wasn't handled by the fatal exception handler then the
AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack
check to fail. Causing the application to crash. So clear the async id
stack manually.

This is only possible if the user: 1) manually calls MakeCallback() or
2) uses async await in the top level. Which will cause _tickCallback()
to fire before bootstrap finishes executing.

The following example shows how the application can fail due to
exceeding the maximum call stack while using async await:

    async function fn() {
      fn();
      throw new Error();
    }
    (async function() { await fn(); })();

If this occurs during bootstrap then the application will pring the
following warning a number of times then exit with a status of 0:

    (node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error

Here's the same recursive call done after enabling a new AsyncHook() the
following will print instead of the above warning and exit with a
non-zero code (currently it's 7 because of how node::FatalException
assigns error codes based on where the failure happened):

    script.js:25
    async function fn() {
                     ^
    RangeError: Maximum call stack size exceeded
        at <anonymous>
        at fn (script.js:25:18)
        at fn (script.js:26:3)
        ....

This has to do with how Promises lazily enable PromiseHook if an
AsyncHook() is enabled. Whether these need to be made uniform is outside
the scope of this commit

Fixes: #15448
PR-URL: #15553
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this issue Sep 29, 2017
If bootstrap throws and if ids are added to the async id stack and if
the exception wasn't handled by the fatal exception handler then the
AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack
check to fail. Causing the application to crash. So clear the async id
stack manually.

This is only possible if the user: 1) manually calls MakeCallback() or
2) uses async await in the top level. Which will cause _tickCallback()
to fire before bootstrap finishes executing.

The following example shows how the application can fail due to
exceeding the maximum call stack while using async await:

    async function fn() {
      fn();
      throw new Error();
    }
    (async function() { await fn(); })();

If this occurs during bootstrap then the application will pring the
following warning a number of times then exit with a status of 0:

    (node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error

Here's the same recursive call done after enabling a new AsyncHook() the
following will print instead of the above warning and exit with a
non-zero code (currently it's 7 because of how node::FatalException
assigns error codes based on where the failure happened):

    script.js:25
    async function fn() {
                     ^
    RangeError: Maximum call stack size exceeded
        at <anonymous>
        at fn (script.js:25:18)
        at fn (script.js:26:3)
        ....

This has to do with how Promises lazily enable PromiseHook if an
AsyncHook() is enabled. Whether these need to be made uniform is outside
the scope of this commit

Fixes: #15448
PR-URL: #15553
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit to addaleax/ayo that referenced this issue Sep 30, 2017
If bootstrap throws and if ids are added to the async id stack and if
the exception wasn't handled by the fatal exception handler then the
AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack
check to fail. Causing the application to crash. So clear the async id
stack manually.

This is only possible if the user: 1) manually calls MakeCallback() or
2) uses async await in the top level. Which will cause _tickCallback()
to fire before bootstrap finishes executing.

The following example shows how the application can fail due to
exceeding the maximum call stack while using async await:

    async function fn() {
      fn();
      throw new Error();
    }
    (async function() { await fn(); })();

If this occurs during bootstrap then the application will pring the
following warning a number of times then exit with a status of 0:

    (node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error

Here's the same recursive call done after enabling a new AsyncHook() the
following will print instead of the above warning and exit with a
non-zero code (currently it's 7 because of how node::FatalException
assigns error codes based on where the failure happened):

    script.js:25
    async function fn() {
                     ^
    RangeError: Maximum call stack size exceeded
        at <anonymous>
        at fn (script.js:25:18)
        at fn (script.js:26:3)
        ....

This has to do with how Promises lazily enable PromiseHook if an
AsyncHook() is enabled. Whether these need to be made uniform is outside
the scope of this commit

Fixes: nodejs/node#15448
PR-URL: nodejs/node#15553
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit to addaleax/ayo that referenced this issue Sep 30, 2017
If bootstrap throws and if ids are added to the async id stack and if
the exception wasn't handled by the fatal exception handler then the
AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack
check to fail. Causing the application to crash. So clear the async id
stack manually.

This is only possible if the user: 1) manually calls MakeCallback() or
2) uses async await in the top level. Which will cause _tickCallback()
to fire before bootstrap finishes executing.

The following example shows how the application can fail due to
exceeding the maximum call stack while using async await:

    async function fn() {
      fn();
      throw new Error();
    }
    (async function() { await fn(); })();

If this occurs during bootstrap then the application will pring the
following warning a number of times then exit with a status of 0:

    (node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error

Here's the same recursive call done after enabling a new AsyncHook() the
following will print instead of the above warning and exit with a
non-zero code (currently it's 7 because of how node::FatalException
assigns error codes based on where the failure happened):

    script.js:25
    async function fn() {
                     ^
    RangeError: Maximum call stack size exceeded
        at <anonymous>
        at fn (script.js:25:18)
        at fn (script.js:26:3)
        ....

This has to do with how Promises lazily enable PromiseHook if an
AsyncHook() is enabled. Whether these need to be made uniform is outside
the scope of this commit

Fixes: nodejs/node#15448
PR-URL: nodejs/node#15553
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 3, 2017
If bootstrap throws and if ids are added to the async id stack and if
the exception wasn't handled by the fatal exception handler then the
AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack
check to fail. Causing the application to crash. So clear the async id
stack manually.

This is only possible if the user: 1) manually calls MakeCallback() or
2) uses async await in the top level. Which will cause _tickCallback()
to fire before bootstrap finishes executing.

The following example shows how the application can fail due to
exceeding the maximum call stack while using async await:

    async function fn() {
      fn();
      throw new Error();
    }
    (async function() { await fn(); })();

If this occurs during bootstrap then the application will pring the
following warning a number of times then exit with a status of 0:

    (node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error

Here's the same recursive call done after enabling a new AsyncHook() the
following will print instead of the above warning and exit with a
non-zero code (currently it's 7 because of how node::FatalException
assigns error codes based on where the failure happened):

    script.js:25
    async function fn() {
                     ^
    RangeError: Maximum call stack size exceeded
        at <anonymous>
        at fn (script.js:25:18)
        at fn (script.js:26:3)
        ....

This has to do with how Promises lazily enable PromiseHook if an
AsyncHook() is enabled. Whether these need to be made uniform is outside
the scope of this commit

Fixes: #15448
PR-URL: #15553
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 23, 2017
PR-URL: #15454
Ref: #14387
Ref: #14722
Ref: #14717
Ref: #15448
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
PR-URL: nodejs/node#15454
Ref: nodejs/node#14387
Ref: nodejs/node#14722
Ref: nodejs/node#14717
Ref: nodejs/node#15448
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
PR-URL: nodejs/node#15454
Ref: nodejs/node#14387
Ref: nodejs/node#14722
Ref: nodejs/node#14717
Ref: nodejs/node#15448
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

No branches or pull requests

6 participants