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.

Sign up for GitHub

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

Jump to bottom

investigate flaky parallel/test-crypto-dh-leak #21076

Closed
Trott opened this issue Jun 1, 2018 · 9 comments
Closed

investigate flaky parallel/test-crypto-dh-leak #21076

Trott opened this issue Jun 1, 2018 · 9 comments
Labels
crypto Issues and PRs related to the crypto subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI.

Comments

@Trott
Copy link
Member

Trott commented Jun 1, 2018

  • Version: v11.0.0-pre (master)
  • Platform: ubuntu1604_sharedlibs_debug_x64
  • Subsystem: test crypto

https://ci.nodejs.org/job/node-test-commit-linux-containered/4846/nodes=ubuntu1604_sharedlibs_debug_x64/console

03:13:14 not ok 414 parallel/test-crypto-dh-leak
03:13:14   ---
03:13:14   duration_ms: 2.717
03:13:14   severity: fail
03:13:14   exitcode: 1
03:13:14   stack: |-
03:13:14     assert.js:270
03:13:14         throw err;
03:13:14         ^
03:13:14     
03:13:14     AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
03:13:14     
03:13:14       assert(after - before < 5 << 20)
03:13:14     
03:13:14         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/test/parallel/test-crypto-dh-leak.js:26:1)
03:13:14         at Module._compile (internal/modules/cjs/loader.js:702:30)
03:13:14         at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
03:13:14         at Module.load (internal/modules/cjs/loader.js:612:32)
03:13:14         at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
03:13:14         at Function.Module._load (internal/modules/cjs/loader.js:543:3)
03:13:14         at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)
03:13:14         at startup (internal/bootstrap/node.js:261:19)
03:13:14         at bootstrapNodeJSCore (internal/bootstrap/node.js:595:3)
03:13:14   ...
@Trott Trott added crypto Issues and PRs related to the crypto subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Jun 1, 2018
@Trott
Copy link
Member Author

Trott commented Jun 1, 2018

By the way, that was the fourth time in a row that happened on test-digitalocean-ubuntu1604_sharedlibs_container-x64-7. Other three are at:

https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_debug_x64/4846/console
https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_debug_x64/4845/console
https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_debug_x64/4841/console

@Trott
Copy link
Member Author

Trott commented Jun 1, 2018

@nodejs/crypto @nodejs/build @nodejs/testing

@Trott
Copy link
Member Author

Trott commented Jun 1, 2018

This comment above the assertion that's failing suggests that this may be inherently unreliable?

// RSS should stay the same, ceteris paribus, but allow for
// some slop because V8 mallocs memory during execution.

@ryzokuken
Copy link
Contributor

@Trott if definitely doesn't fail on MacOS. I'll try to look into it.

@ryzokuken
Copy link
Contributor 

// RSS should stay the same, ceteris paribus, but allow for
// some slop because V8 mallocs memory during execution.

Shouldn't this comment be improved? Of all things: why is there latin in there?

@bnoordhuis
Copy link
Member

Because being a programmer doesn't mean you can't be well-versed in the liberal arts too. Ceteris paribus means "all other things being equal" but with fewer words.

Is the ./configure --debug buildbot the only one where it fails? If yes, I would guess that V8's debug bookkeeping slanting the numbers.

@bnoordhuis bnoordhuis mentioned this issue Jun 1, 2018
Closed
@tniessen
Copy link
Member

tniessen commented Jun 1, 2018

We might be able to circumvent that by massively increasing the number of iterations and slightly increasing the amount of permitted RSS difference.

@Trott
Copy link
Member Author

Trott commented Jun 1, 2018

Other options:

We could check process.config.target_defaults.default_configuration and skip the test if it's not 'Release'. h/t @danbev

Or @tniessen is suggesting we could remove the test entirely as it was for a very specific regression.

Thoughts? @bnoordhuis

@tniessen
Copy link
Member

tniessen commented Jun 1, 2018

On my maschine, the debug build allocates 4.8MB of the 5MB limit, but that value does not increase when I increase the number of iterations, so #21076 (comment) could work.

tniessen pushed a commit to tniessen/node that referenced this issue Jun 1, 2018
@bnoordhuis @tniessen
test: log before and after RSS in memory leak test
eebd5b7 
Refs: nodejs#21076
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Jun 2, 2018
@bnoordhuis
test: log before and after RSS in memory leak test
cb3989f 
Refs: nodejs#21076
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Jun 2, 2018
@bnoordhuis
test: increase slop limit in memory leak test
1569bf4 
It transpires that the extra bookkeeping in debug builds sometimes makes
the increase in RSS go _just_ over the 5 MB limit, by fewer than 100 kB.
Double the limit so we hopefully don't run into it any time again soon.

The memory leak it tests for was one where RSS grew by hundreds of
megabytes over the lifetime of the test; 5 vs. 10 MB is insignificant.

Fixes: nodejs#21076
Trott pushed a commit to Trott/io.js that referenced this issue Jun 3, 2018
@bnoordhuis @Trott
test: log before and after RSS in memory leak test
b557a3b 
Refs: nodejs#21076

PR-URL: nodejs#21080
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott Trott closed this as completed in 997e97d Jun 3, 2018
MylesBorins pushed a commit that referenced this issue Jun 6, 2018
@bnoordhuis @MylesBorins
test: log before and after RSS in memory leak test
fda8654 
Refs: #21076

PR-URL: #21080
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this issue Jun 6, 2018
@bnoordhuis @MylesBorins
test: increase slop limit in memory leak test
c60810a 
It transpires that the extra bookkeeping in debug builds sometimes makes
the increase in RSS go _just_ over the 5 MB limit, by fewer than 100 kB.
Double the limit so we hopefully don't run into it any time again soon.

The memory leak it tests for was one where RSS grew by hundreds of
megabytes over the lifetime of the test; 5 vs. 10 MB is insignificant.

Fixes: #21076

PR-URL: #21080
Refs: #21076
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bnoordhuis @Trott @tniessen @ryzokuken