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

test: fix test failure on aix #20940

Closed
wants to merge 1 commit into from
Closed

Conversation

BridgeAR
Copy link
Member

This makes sure there is enough stack space on different systems. 75 should definitely be enough on all systems.

Alternative to #20927

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

This makes sure there is enough stack space on different systems.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 24, 2018
@gireeshpunathil
Copy link
Member

75 works in my AIX box. Further thinking on it, --stack_size is not a function of the OS thread stack size, instead the amount we are offering to v8 - which means this should be platform agnostic from the native stack perspective, but a function of how much frame size each JS code has consumed already, which can be platform specific. (An explanation for the platform different behaviors, if anyone is interested).

@richardlau
Copy link
Member

I'm not going to block this, but I am wary of magic values in test files. Can you guarantee 75 will be a safe value in the future? It feels like setting the stack_size introduces an inherent flakiness -- we picked 50 previously and that turned out to be insufficient.

@gireeshpunathil
Copy link
Member

I guess the number which we are taking about is the amount of kilobytes that is required for the frames to execute the methods in this call chain:

RangeError: Maximum call stack size exceeded
    at runInThisContext (bootstrap_node.js:496:20)
    at NativeModule.compile (bootstrap_node.js:591:18)
    at NativeModule.require (bootstrap_node.js:541:18)
    at module.js:30:12
    at NativeModule.compile (bootstrap_node.js:596:7)
    at Function.NativeModule.require (bootstrap_node.js:541:18)
    at setupGlobalConsole (bootstrap_node.js:308:33)
    at startup (bootstrap_node.js:70:7)
    at bootstrap_node.js:608:3

if those sequences change and start consuming more stack space, 75 will become insufficient too.

@BridgeAR
Copy link
Member Author

@richardlau I was surprised that it turned out as insufficient because I expected it to be a fixed value on all platforms. However, it would be very surprising if the new value would not be sufficient anymore (also in future cases) because that could only happen in case V8 changes something significantly to the worse or we add multiple extra layers and that is also very unlikely.

We should always realize if the value is insufficient on a CI run and I guess the aix failure originally just slipped through.

@Trott
Copy link
Member

Trott commented May 24, 2018

FWIW, this was also observed by @danbev on a macOS laptop. Not sure if that warrants changing the description from saying AIX or not.

Also in the "might not really require amending the commit message" department: The stylization of AIX is all caps.

@@ -15,7 +15,7 @@ const { spawnSync } = require('child_process');

const ret = spawnSync(
process.execPath,
['--stack_size=50', __filename, 'async']
['--stack_size=75', __filename, 'async']
);
assert.strictEqual(ret.status, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, I think this would have been more easily debuggable if this assertion had come last and the assertion on L22 would print out stderr in its failure message – maybe we can fix up the test for that as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, actually, I had a branch to fix this and then didn't open a PR, but maybe it can be tacked onto this. IIRC, the change was this:

assert.strictEqual(ret.status, 0,
                   `EXIT CODE: ${ret.status}, STDERR: ${ret.stderr}`);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #20948 so that this one doesn't get delayed with any bike-shedding or CI re-running.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 25, 2018
@BridgeAR
Copy link
Member Author

@BridgeAR BridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label May 25, 2018
@BridgeAR
Copy link
Member Author

Please +1 to fast track this.

@joyeecheung
Copy link
Member

There were some infra issues with the last CI. New CI: https://ci.nodejs.org/job/node-test-pull-request/15113/

@Trott
Copy link
Member

Trott commented May 28, 2018

node-test-commit-osx re-run: https://ci.nodejs.org/job/node-test-commit-osx/18888/

@Trott Trott removed the fast-track PRs that do not need to wait for 48 hours to land. label May 28, 2018
@Trott
Copy link
Member

Trott commented May 28, 2018

(Removed fast-track label because it's been open for four days and has three approvals, so all it needs is a clean CI.)

Trott pushed a commit to Trott/io.js that referenced this pull request May 28, 2018
This makes sure there is enough stack space on different systems.

PR-URL: nodejs#20940
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@Trott
Copy link
Member

Trott commented May 28, 2018

Landed in cfc3866

@Trott Trott closed this May 28, 2018
@Trott Trott mentioned this pull request May 28, 2018
1 task
MylesBorins pushed a commit that referenced this pull request May 29, 2018
This makes sure there is enough stack space on different systems.

PR-URL: #20940
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 29, 2018
danbev added a commit to danbev/node that referenced this pull request Oct 31, 2018
Currently, when building with --debug
test-async-wrap-pop-id-during-load fails on macosx with the following
error:

$ out/Debug/node test/parallel/test-async-wrap-pop-id-during-load.js
assert.js:86
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: EXIT CODE: 1, STDERR:
internal/bootstrap/loaders.js:275
      const script = new ContextifyScript(
                     ^

RangeError: Maximum call stack size exceeded
    at NativeModule.compile (internal/bootstrap/loaders.js:275:22)
    at NativeModule.require (internal/bootstrap/loaders.js:168:18)
    at assert.js:31:43
    at NativeModule.compile (internal/bootstrap/loaders.js:299:7)
    at NativeModule.require (internal/bootstrap/loaders.js:168:18)
    at internal/process/main_thread_only.js:23:16
    at NativeModule.compile (internal/bootstrap/loaders.js:299:7)
    at Function.NativeModule.require
      (internal/bootstrap/loaders.js:168:18)
    at startup (internal/bootstrap/node.js:58:38)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)

    at Object.<anonymous>
       (/node/test/parallel/test-async-wrap-pop-id-during-load.js:21:8)
    at Module._compile (internal/modules/cjs/loader.js:707:30)
    at Object.Module._extensions..js
      (internal/modules/cjs/loader.js:718:10)
    at Module.load (internal/modules/cjs/loader.js:605:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:544:12)
    at Function.Module._load (internal/modules/cjs/loader.js:536:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:760:12)
    at startup (internal/bootstrap/node.js:308:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)

This commit suggests increasing the stack_size to 80.

Refs: nodejs#20940
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 3, 2018
Currently, when building with --debug
test-async-wrap-pop-id-during-load fails on macosx with the following
error:

$ out/Debug/node test/parallel/test-async-wrap-pop-id-during-load.js
assert.js:86
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: EXIT CODE: 1, STDERR:
internal/bootstrap/loaders.js:275
      const script = new ContextifyScript(
                     ^

RangeError: Maximum call stack size exceeded
    at NativeModule.compile (internal/bootstrap/loaders.js:275:22)
    at NativeModule.require (internal/bootstrap/loaders.js:168:18)
    at assert.js:31:43
    at NativeModule.compile (internal/bootstrap/loaders.js:299:7)
    at NativeModule.require (internal/bootstrap/loaders.js:168:18)
    at internal/process/main_thread_only.js:23:16
    at NativeModule.compile (internal/bootstrap/loaders.js:299:7)
    at Function.NativeModule.require
      (internal/bootstrap/loaders.js:168:18)
    at startup (internal/bootstrap/node.js:58:38)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)

    at Object.<anonymous>
       (/node/test/parallel/test-async-wrap-pop-id-during-load.js:21:8)
    at Module._compile (internal/modules/cjs/loader.js:707:30)
    at Object.Module._extensions..js
      (internal/modules/cjs/loader.js:718:10)
    at Module.load (internal/modules/cjs/loader.js:605:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:544:12)
    at Function.Module._load (internal/modules/cjs/loader.js:536:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:760:12)
    at startup (internal/bootstrap/node.js:308:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)

This commit suggests increasing the stack_size to 80.

Refs: nodejs#20940
PR-URL: nodejs#23996
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Nov 3, 2018
Currently, when building with --debug
test-async-wrap-pop-id-during-load fails on macosx with the following
error:

$ out/Debug/node test/parallel/test-async-wrap-pop-id-during-load.js
assert.js:86
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: EXIT CODE: 1, STDERR:
internal/bootstrap/loaders.js:275
      const script = new ContextifyScript(
                     ^

RangeError: Maximum call stack size exceeded
    at NativeModule.compile (internal/bootstrap/loaders.js:275:22)
    at NativeModule.require (internal/bootstrap/loaders.js:168:18)
    at assert.js:31:43
    at NativeModule.compile (internal/bootstrap/loaders.js:299:7)
    at NativeModule.require (internal/bootstrap/loaders.js:168:18)
    at internal/process/main_thread_only.js:23:16
    at NativeModule.compile (internal/bootstrap/loaders.js:299:7)
    at Function.NativeModule.require
      (internal/bootstrap/loaders.js:168:18)
    at startup (internal/bootstrap/node.js:58:38)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)

    at Object.<anonymous>
       (/node/test/parallel/test-async-wrap-pop-id-during-load.js:21:8)
    at Module._compile (internal/modules/cjs/loader.js:707:30)
    at Object.Module._extensions..js
      (internal/modules/cjs/loader.js:718:10)
    at Module.load (internal/modules/cjs/loader.js:605:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:544:12)
    at Function.Module._load (internal/modules/cjs/loader.js:536:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:760:12)
    at startup (internal/bootstrap/node.js:308:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)

This commit suggests increasing the stack_size to 80.

Refs: #20940
PR-URL: #23996
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Nov 29, 2018
Currently, when building with --debug
test-async-wrap-pop-id-during-load fails on macosx with the following
error:

$ out/Debug/node test/parallel/test-async-wrap-pop-id-during-load.js
assert.js:86
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: EXIT CODE: 1, STDERR:
internal/bootstrap/loaders.js:275
      const script = new ContextifyScript(
                     ^

RangeError: Maximum call stack size exceeded
    at NativeModule.compile (internal/bootstrap/loaders.js:275:22)
    at NativeModule.require (internal/bootstrap/loaders.js:168:18)
    at assert.js:31:43
    at NativeModule.compile (internal/bootstrap/loaders.js:299:7)
    at NativeModule.require (internal/bootstrap/loaders.js:168:18)
    at internal/process/main_thread_only.js:23:16
    at NativeModule.compile (internal/bootstrap/loaders.js:299:7)
    at Function.NativeModule.require
      (internal/bootstrap/loaders.js:168:18)
    at startup (internal/bootstrap/node.js:58:38)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)

    at Object.<anonymous>
       (/node/test/parallel/test-async-wrap-pop-id-during-load.js:21:8)
    at Module._compile (internal/modules/cjs/loader.js:707:30)
    at Object.Module._extensions..js
      (internal/modules/cjs/loader.js:718:10)
    at Module.load (internal/modules/cjs/loader.js:605:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:544:12)
    at Function.Module._load (internal/modules/cjs/loader.js:536:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:760:12)
    at startup (internal/bootstrap/node.js:308:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)

This commit suggests increasing the stack_size to 80.

Refs: #20940
PR-URL: #23996
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Currently, when building with --debug
test-async-wrap-pop-id-during-load fails on macosx with the following
error:

$ out/Debug/node test/parallel/test-async-wrap-pop-id-during-load.js
assert.js:86
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: EXIT CODE: 1, STDERR:
internal/bootstrap/loaders.js:275
      const script = new ContextifyScript(
                     ^

RangeError: Maximum call stack size exceeded
    at NativeModule.compile (internal/bootstrap/loaders.js:275:22)
    at NativeModule.require (internal/bootstrap/loaders.js:168:18)
    at assert.js:31:43
    at NativeModule.compile (internal/bootstrap/loaders.js:299:7)
    at NativeModule.require (internal/bootstrap/loaders.js:168:18)
    at internal/process/main_thread_only.js:23:16
    at NativeModule.compile (internal/bootstrap/loaders.js:299:7)
    at Function.NativeModule.require
      (internal/bootstrap/loaders.js:168:18)
    at startup (internal/bootstrap/node.js:58:38)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)

    at Object.<anonymous>
       (/node/test/parallel/test-async-wrap-pop-id-during-load.js:21:8)
    at Module._compile (internal/modules/cjs/loader.js:707:30)
    at Object.Module._extensions..js
      (internal/modules/cjs/loader.js:718:10)
    at Module.load (internal/modules/cjs/loader.js:605:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:544:12)
    at Function.Module._load (internal/modules/cjs/loader.js:536:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:760:12)
    at startup (internal/bootstrap/node.js:308:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)

This commit suggests increasing the stack_size to 80.

Refs: #20940
PR-URL: #23996
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 3, 2018
Currently, when building with --debug
test-async-wrap-pop-id-during-load fails on macosx with the following
error:

$ out/Debug/node test/parallel/test-async-wrap-pop-id-during-load.js
assert.js:86
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: EXIT CODE: 1, STDERR:
internal/bootstrap/loaders.js:275
      const script = new ContextifyScript(
                     ^

RangeError: Maximum call stack size exceeded
    at NativeModule.compile (internal/bootstrap/loaders.js:275:22)
    at NativeModule.require (internal/bootstrap/loaders.js:168:18)
    at assert.js:31:43
    at NativeModule.compile (internal/bootstrap/loaders.js:299:7)
    at NativeModule.require (internal/bootstrap/loaders.js:168:18)
    at internal/process/main_thread_only.js:23:16
    at NativeModule.compile (internal/bootstrap/loaders.js:299:7)
    at Function.NativeModule.require
      (internal/bootstrap/loaders.js:168:18)
    at startup (internal/bootstrap/node.js:58:38)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)

    at Object.<anonymous>
       (/node/test/parallel/test-async-wrap-pop-id-during-load.js:21:8)
    at Module._compile (internal/modules/cjs/loader.js:707:30)
    at Object.Module._extensions..js
      (internal/modules/cjs/loader.js:718:10)
    at Module.load (internal/modules/cjs/loader.js:605:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:544:12)
    at Function.Module._load (internal/modules/cjs/loader.js:536:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:760:12)
    at startup (internal/bootstrap/node.js:308:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)

This commit suggests increasing the stack_size to 80.

Refs: #20940
PR-URL: #23996
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR deleted the fix-aix-failure branch January 20, 2020 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants