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

Investigate flaky test-crypto-timing-safe-equal-benchmarks #25984

Closed
Trott opened this issue Feb 7, 2019 · 6 comments
Closed

Investigate flaky test-crypto-timing-safe-equal-benchmarks #25984

Trott opened this issue Feb 7, 2019 · 6 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. test Issues and PRs related to the tests.

Comments

@Trott
Copy link
Member

Trott commented Feb 7, 2019

https://ci.nodejs.org/job/node-test-commit-custom-suites/864/default/console

test-rackspace-ubuntu1604-x64-1

00:05:42 not ok 62 pummel/test-crypto-timing-safe-equal-benchmarks
00:05:42   ---
00:05:42   duration_ms: 27.177
00:05:42   severity: fail
00:05:42   exitcode: 1
00:05:42   stack: |-
00:05:42     assert.js:351
00:05:42         throw err;
00:05:42         ^
00:05:42     
00:05:42     AssertionError [ERR_ASSERTION]: timingSafeEqual should not leak information from its execution time (t=4.860941889108833)
00:05:42         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-custom-suites/default/test/pummel/test-crypto-timing-safe-equal-benchmarks.js:106:1)
00:05:42         at Module._compile (internal/modules/cjs/loader.js:735:30)
00:05:42         at Object.Module._extensions..js (internal/modules/cjs/loader.js:746:10)
00:05:42         at Module.load (internal/modules/cjs/loader.js:627:32)
00:05:42         at tryModuleLoad (internal/modules/cjs/loader.js:570:12)
00:05:42         at Function.Module._load (internal/modules/cjs/loader.js:562:3)
00:05:42         at Function.Module.runMain (internal/modules/cjs/loader.js:798:12)
00:05:42         at internal/main/run_main_module.js:27:11
00:05:42   ...
@Trott Trott added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Feb 7, 2019
@Trott
Copy link
Member Author

Trott commented Feb 7, 2019

@nodejs/crypto

@Trott
Copy link
Member Author

Trott commented Feb 7, 2019

@not-an-aardvark

@Trott Trott added crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests. labels Feb 7, 2019
@Trott
Copy link
Member Author

Trott commented Feb 20, 2019

https://ci.nodejs.org/job/node-test-commit-custom-suites/880/default/console

test-rackspace-ubuntu1604-x64-1

00:07:31 not ok 62 pummel/test-crypto-timing-safe-equal-benchmarks
00:07:31   ---
00:07:31   duration_ms: 120.98
00:07:31   severity: fail
00:07:31   exitcode: -15
00:07:31   stack: |-
00:07:31     timeout
00:07:31   ...

@Trott
Copy link
Member Author

Trott commented Feb 20, 2019

Last successful run took 52 seconds. If there's significant randomness involved (which seems possible given that it's crypto), then a two minute timeout might be an expected result from time to time. But I don't actually know.

@Trott
Copy link
Member Author

Trott commented Feb 20, 2019

Last successful run took 52 seconds. If there's significant randomness involved (which seems possible given that it's crypto), then a two minute timeout might be an expected result from time to time. But I don't actually know.

Meh, maybe not. Successful results are pretty consistent in duration. 52 seconds, 53 seconds, 52 seconds....

@Trott
Copy link
Member Author

Trott commented Feb 21, 2019

Cause is #26229.

Trott added a commit to Trott/io.js that referenced this issue Feb 21, 2019
Resetting require.cache() to `Object.create(null)` each time rather than
deleting the specific key results in a 10x improvement in running time.

Fixes: nodejs#25984
Refs: nodejs#26229
@Trott Trott closed this as completed in eb9e6c0 Feb 22, 2019
addaleax pushed a commit that referenced this issue Feb 25, 2019
Using `eval()` rather than `require()`'ing a fixture and deleting it
from the cache results in a roughtly 10x improvement in running time.

Fixes: #25984
Refs: #26229

PR-URL: #26237
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rvagg pushed a commit that referenced this issue Feb 28, 2019
Using `eval()` rather than `require()`'ing a fixture and deleting it
from the cache results in a roughtly 10x improvement in running time.

Fixes: #25984
Refs: #26229

PR-URL: #26237
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant