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: flaky parallel/test-crypto-dh #3881

Closed
bnoordhuis opened this issue Nov 17, 2015 · 15 comments
Closed

test: flaky parallel/test-crypto-dh #3881

bnoordhuis opened this issue Nov 17, 2015 · 15 comments
Labels
crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.

Comments

@bnoordhuis
Copy link
Member

not ok 132 test-crypto-dh.js
# TIMEOUT
  ---
  duration_ms: 120.60

I can reproduce this locally up to a point:

$ time out/Release/node test/parallel/test-crypto-dh.js

real    0m1.264s
user    0m1.254s
sys     0m0.010s

$ time out/Release/node test/parallel/test-crypto-dh.js

real    0m13.104s
user    0m13.096s
sys     0m0.009s

$ time out/Release/node test/parallel/test-crypto-dh.js

real    0m3.611s
user    0m3.603s
sys     0m0.007s

The high variance is because the test has to search for suitable prime numbers:

            |--75.34%-- BN_mod_word
            |          |
            |          |--99.79%-- BN_generate_prime_ex
            |          |          DH_generate_parameters_ex
            |          |          node::crypto::DiffieHellman::New
            |          |          v8::internal::FunctionCallbackArguments::Call
            |          |          v8::internal::Builtin_HandleApiCallConstruct
            |          |          Stub:CEntryStub
            |          |          Builtin:JSConstructStubApi
            |          |          LazyCompile:~DiffieHellman crypto.js:340
            |          |          Builtin:JSConstructStubGeneric
            |          |          LazyCompile:~DiffieHellman crypto.js:340
            |          |          Builtin:ArgumentsAdaptorTrampoline
            |          |          Function:~ /home/bnoordhuis/src/v1.x/test/parallel/test-crypto-dh.js:1
            |          |          Builtin:FunctionApply
            |          |          LazyCompile:~Module._compile module.js:377
            |          |          LazyCompile:~Module._extensions..js module.js:428
            |          |          LazyCompile:~Module.load module.js:345
            |          |          LazyCompile:Module._load module.js:271
            |          |          LazyCompile:~Module.runMain module.js:453
            |          |          LazyCompile:~startup node.js:13
            |          |          Function:~ node.js:10
            |          |          Builtin:JSEntryTrampoline
            |          |          Stub:JSEntryStub
            |          |          v8::internal::Execution::Call
            |          |          v8::Function::Call
            |          |          v8::Function::Call
            |          |          node::LoadEnvironment
            |          |          node::Start
            |          |          __libc_start_main
            |          |          0xc796258d4c544155

I think we either have to give this test (virtually) unlimited running time or move it to test/pummel.

@bnoordhuis bnoordhuis added the test Issues and PRs related to the tests. label Nov 17, 2015
@bnoordhuis
Copy link
Member Author

It looks like test/parallel/test-crypto-binary-default.js is also affected by this.

@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Nov 17, 2015
@Fishrock123
Copy link
Contributor

Both of these also appeared while testing the v5.1.0 release.

  • test/parallel/test-crypto-binary-default.js: [1], [2]
  • test/parallel/test-crypto-dh.js: [1], [2]

@evanlucas
Copy link
Contributor

Was a change recently made that caused these tests to start timing out? I don't remember seeing these failures until pretty recently

@Trott
Copy link
Member

Trott commented Nov 18, 2015

@evanlucas: 11ad744 on Nov. 10.

@evanlucas
Copy link
Contributor

thanks

@Trott
Copy link
Member

Trott commented Nov 18, 2015

Increasing the key size four fold (from 256 to 1024 bytes) as in 11ad744 sure seems like it would explain why the tests might start timing out on Raspberry Pis. Maybe the test can check available memory or CPU or something and select a key size based on that?

@evanlucas
Copy link
Contributor

It looks like the CI was not run for #3758 too

@Trott
Copy link
Member

Trott commented Nov 18, 2015

@evanlucas Are you working on trying to figure a fix for this and I should go about my business elsewhere? Or should I keep poking at this? I would be stoked if you were on it because there are plenty of other flaky tests to investigate that aren't this one. But I'm happy to try to figure something out myself if your work here is done.

(If you try to use node-stress-single-test for this, do read my note at nodejs/build#260 first.)

@evanlucas
Copy link
Contributor

I have looked at it a little, but won't be able to get to it for a few hours. Up to you :]

@Fishrock123
Copy link
Contributor

@Trott Maybe just use the old key sizes on rpi?

@shigeki
Copy link
Contributor

shigeki commented Nov 18, 2015

@jasnell I think we can resolve the timeout error if tests are not running in fips mode. Do we change tests?

diff --git a/test/parallel/test-crypto-binary-default.js b/test/parallel/test-crypto-binary-default.js
index 8695632..305c110 100644
--- a/test/parallel/test-crypto-binary-default.js
+++ b/test/parallel/test-crypto-binary-default.js
@@ -513,7 +513,8 @@ assert.throws(function() {

 // Test Diffie-Hellman with two parties sharing a secret,
 // using various encodings as we go along
-var dh1 = crypto.createDiffieHellman(1024);
+var keylen = common.hasFipsCrypto ? 1024 : 256;
+var dh1 = crypto.createDiffieHellman(keylen);
 var p1 = dh1.getPrime('buffer');
 var dh2 = crypto.createDiffieHellman(p1, 'base64');
 var key1 = dh1.generateKeys();
diff --git a/test/parallel/test-crypto-dh.js b/test/parallel/test-crypto-dh.js
index d93c53e..887bc59 100644
--- a/test/parallel/test-crypto-dh.js
+++ b/test/parallel/test-crypto-dh.js
@@ -11,7 +11,8 @@ var crypto = require('crypto');

 // Test Diffie-Hellman with two parties sharing a secret,
 // using various encodings as we go along
-var dh1 = crypto.createDiffieHellman(1024);
+var keylen = common.hasFipsCrypto ? 1024 : 256;
+var dh1 = crypto.createDiffieHellman(keylen);
 var p1 = dh1.getPrime('buffer');
 var dh2 = crypto.createDiffieHellman(p1, 'buffer');
 var key1 = dh1.generateKeys();

@stefanmb
Copy link
Contributor

@shigeki @Fishrock123 @Trott @jasnell I'll make a PR shortly that restores the old sizes outside of FIPS mode.

@shigeki
Copy link
Contributor

shigeki commented Nov 18, 2015

@stefanmb Thanks.

@stefanmb
Copy link
Contributor

@shigeki There were only two places were 1024-bit long primes were being generated, this PR should resolve the perf issues on rpi: #3902

@jasnell
Copy link
Member

jasnell commented Nov 18, 2015

Landed #3902.

@jasnell jasnell closed this as completed Nov 18, 2015
jasnell pushed a commit that referenced this issue Nov 18, 2015
Generating 1024-bit primes on rpi test machines sometimes
causes timeouts. Avoid this situation by using 256-bit
primes when not running in FIPS mode.

Fixes: #3881
PR-URL: #3902
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
rvagg pushed a commit that referenced this issue Dec 5, 2015
Generating 1024-bit primes on rpi test machines sometimes
causes timeouts. Avoid this situation by using 256-bit
primes when not running in FIPS mode.

Fixes: #3881
PR-URL: #3902
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
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. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

8 participants