parallel/test-tls-server-verify is awfully slow on Windows

[jbergstroem]

Running this test on one of our jenkins windows bots consistently takes more than 35 seconds, while taking 0.6s on my (os x) desktop.

[mathiask88]

I think the cause for the long runtime is in OpenSSL. The s_client on windows generates extra entropy by loading screen memory and this takes ~1-2s on each spawn. I don't get why they are not using something like windows CryptGenRandom() function but I'm no openssl or even crypto expert so correct me if I am wrong.

[jbergstroem]

@mathiask88 thanks for digging that up. Seems to have been around for a while.

[trevnorris]

/cc @indutny @piscisaureus on using CryptGetRandom()

[piscisaureus]

@mathiask88 The test has always been slow for me, but I never got any further than figuring out that the openssl command-line client is really slow. Thanks for digging this up.

[mathiask88]

@jbergstroem Yes and I think it is not necessary. The client runs app_RAND_load_file() that returns on windows something like C:\.rnd and tries to read that entropy file. If this fails RAND_status() gets called and if this is called for the first time openssl gets initialized with RAND_poll() and that function calls CryptGetRandom() on Windows NT+ or Windows CE 3.0+ anyway. So I think RAND_screen() is a relict where OpenSSL didn't use an OS-based seed initialization.

But I tested a bit more and it seems that the screen loading is not the bottleneck here. It doesn't take ~1s as I thought. If I run a server like in the test and then a *.bat file with echo QUIT | openssl-cli.exe s_client -connect 127.0.0.1:12346 the openssl-cli takes about 2s to exit after _(un)authed.

[orangemocha]

The test is slow on Windows because of two 1 second delays in the Windows version of openssl-cli.exe. We have reported the issue to openssl: http://rt.openssl.org/Ticket/Display.html?user=guest&pass=guest&id=3849

Now working on a workaround, modifying the test to let all those test cases run in parallel instead of serialized, so that it finishes quickly in spite of the openssl-cli delay.

We will submit the change to io.js as well as Node.

[Fishrock123]

We will submit the change to io.js as well as Node.

Awesome, thanks!

[orangemocha]

Do you folks mind reviewing the change here: nodejs/node-v0.x-archive#25368, to make sure we have a consistent +1? I can then cherry pick the commits to io.js in another PR. Thanks!

[rvagg]

/cc @nodejs/crypto to @orangemocha's comment above, sounds positive to me but could do with a word of encouragement from someone in the crypto team

[shigeki]

I looked at this and found that we can have one more improvement to use child.kill() not to wait for child processes to be ended. shigeki@ad45f88

RAND_screen() on my Windows takes more than one second. I think we need not to have a good client randomness in this test. Adding a -no-rand-screen option for openssl s_client on Windows is one of ideas to remove this overhead. shigeki@12be7ec and shigeki@dae36fb

The benchmark results are

Current
TotalSeconds      : 86.2152129

joyent/node#25368 
TotalSeconds      : 41.0233476

joyent/node#25368 + child.kill()
TotalSeconds      : 33.9199469

joyent/node#25368 + child.kill() + -no-rand-screen
TotalSeconds      : 9.5351881

9.5 seconds are still slow compared to Unix but it seems to be enough good for CI.

[jbergstroem]

I'm all for --no-rand-screen; this openssl binary isn't used for anything other than verifying our test suite.

[orangemocha]

Regarding using child.kill(), I wonder if this changes the test in way that could limit its effectiveness in catching bugs. But I will add shigeki@ad45f88 to the PR and let people who are more experienced in this area provide feedback.

Regarding, --no-rand-screen it sounds like a wonderful feature. But it would be a floating patch on a dependency, and I wonder if that would be appropriate here. In node we usually tend to limit those floating patches to changes that have already been made in the upstream project, or are high priority and at least have a clear path for being accepted upstream.

[orangemocha]

Updated nodejs/node-v0.x-archive#25368

[shigeki]

@orangemocha The patch of -no-rand-screen would not be accepted to the upstream because it can not be applied in a general use. This fix was made carefully to be as small as minimum and limited to have changes of only openssl-cli.exe binary so that libopenssl.a linked to node is nothing to do with it.

There are private patches which are already applied in deps/openssl. For openssl-1.0.2a case, these are
https://github.com/nodejs/io.js/blob/master/deps/openssl/doc/UPGRADING.md#2-apply-private-patches
But I agree that those private patches should be kept in minimum.

[orangemocha]

@shigeki : added all your commits to nodejs/node-v0.x-archive#25368 to get feedback there.

The PR at nodejs/node-v0.x-archive#25368 is ready for review. cc @nodejs/crypto

[shigeki]

@indutny @bnoordhuis Could you give a comment on shigeki@12be7ec if we can add it into deps/openssl? It surely saves our time to wait for CI results on Windows.

[bnoordhuis]

@orangemocha @shigeki If you want your changes to get landed in io.js, can you file them as a PR here?

[orangemocha]

sure @bnoordhuis , against v1.x?

[bnoordhuis]

@orangemocha master (or next if you want it to go into v3.x.) If it needs to get back-ported to a v1.x. release, please mention that in the pull request.

[orangemocha]

io.js PR is here: #1836