test,win: speedup tls-server-verify #1836

orangemocha · 2015-05-29T17:11:34Z

Addresses #1461 (and nodejs/node-v0.x-archive#25279 once ported there).

With the -no-rand-screen changes, the test is now much much faster (less than 1s on my machine). I am not sure however if speeding up a test is a good justification for adding a floating patch in openssl. I added all the commits in here to get more feedback.

orangemocha · 2015-05-29T17:11:52Z

cc @nodejs/crypto

piscisaureus · 2015-05-29T17:24:43Z

<3

rvagg · 2015-05-30T03:14:29Z

what are the chances of getting these upstreamed to openssl?

shigeki · 2015-05-30T04:21:07Z

@rvagg No chance because this makes a client connection be in a low entropy state. This change only applied to openssl s_client command on Windows and no affects to libopenssl.a so that iojs has a good entropy state.

mathiask88 · 2015-05-30T08:32:33Z

@shigeki Why is it a low entropy? RAND_poll(CryptGenRandom) still gets called and for the CI the screen content is always the same, so not much entropy added in this case.

bnoordhuis · 2015-05-30T11:36:35Z

deps/openssl/openssl/apps/s_client.c

@@ -446,6 +447,10 @@ static void sc_usage(void)
               " -keymatexport label   - Export keying material using label\n");
    BIO_printf(bio_err,
               " -keymatexportlen len  - Export len bytes of keying material (default 20)\n");
+#ifdef OPENSSL_SYS_WINDOWS
+    BIO_printf(bio_err,
+               " -no-rand-screen  - Do not use RAND_screen() to initialize random state\n");


Nitpick: name it -no_rand_screen for consistency with other options.

I agree it. Fixed.

shigeki · 2015-06-01T09:44:23Z

@mathiask88 I should have written it as "lower than the current". I don't think it is absolutely low for RAND_add with CryptGenRandom but I can't say it is enough high without RAND_screen for general uses. In CI, I agree that we need not it for this server-verify tests.

shigeki · 2015-06-01T17:02:20Z

@orangemocha According to Ben's comments, my commits were updated in shigeki@833b236 , shigeki@9f0f7c3 and shigeki@2ff517e .

OpenSSL s_client introduces some delay on Windows. With all clients running sequentially, this delay is big enough to break CI. This fix runs the clients in parallel (unless the test includes renegotiation), reducing the total run time. (cherry picked from commit 12d4ea3c8ae7f8011de9ca9f50bdc08ee8cfb8bf)

Different servers must use different ports. Since we can count only on common.PORT and common.PORT+1, run only 2 servers in parallel. (cherry picked from commit efb0075ee0e1f4123974fe5b74ded8727bad4a9e)

When running in parallel, it is not easy to identify what server and client failed when the test fails. This adds identifiers to all lines of console output. (cherry picked from commit 29c651522df2f47c4360660264eb857fb6232b23)

For better performance of the test, the parent kills child processes so as not to wait them to be ended. (cherry picked from commit 833b23636045f7afc929196139021630a390391a)

In openssl s_client on Windows, RAND_screen() is invoked to initialize random state but it takes several seconds in each connection. This added -no_rand_screen to openssl s_client on Windows to skip RAND_screen() and gets a better performance in the unit test of test-tls-server-verify. Do not enable this except to use in the unit test. (cherry picked from commit 9f0f7c38e6df975dd39735d0e9ef968076369c74)

This improves the performance of openssl s_client on Windows and gains several seconds to finish test-tls-server-verify. (cherry picked from commit 2ff517e0e410ea33ba5a3d289a82fc315d120e8e)

joaocgreis · 2015-06-02T10:46:19Z

@bnoordhuis Thanks for your comments, I updated my commits. I also picked the updated commits from @shigeki .

bnoordhuis · 2015-06-02T11:00:18Z

LGTM. CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/750/

orangemocha · 2015-06-03T15:19:10Z

Thanks, @bnoordhuis . I will land this shortly.

OpenSSL s_client introduces some delay on Windows. With all clients running sequentially, this delay is big enough to break CI. This fix runs the clients in parallel (unless the test includes renegotiation), reducing the total run time. Fixes: nodejs#1461 PR-URL: nodejs#1836 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Different servers must use different ports. Since we can count only on common.PORT and common.PORT+1, run only 2 servers in parallel. Fixes: nodejs#1461 PR-URL: nodejs#1836 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

In openssl s_client on Windows, RAND_screen() is invoked to initialize random state but it takes several seconds in each connection. This added -no_rand_screen to openssl s_client on Windows to skip RAND_screen() and gets a better performance in the unit test of test-tls-server-verify. Do not enable this except to use in the unit test. Fixes: #1461 PR-URL: #1836 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

In openssl s_client on Windows, RAND_screen() is invoked to initialize random state but it takes several seconds in each connection. This added -no_rand_screen to openssl s_client on Windows to skip RAND_screen() and gets a better performance in the unit test of test-tls-server-verify. Do not enable this except to use in the unit test. Fixes: nodejs#1461 PR-URL: nodejs#1836 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

In openssl s_client on Windows, RAND_screen() is invoked to initialize random state but it takes several seconds in each connection. This added -no_rand_screen to openssl s_client on Windows to skip RAND_screen() and gets a better performance in the unit test of test-tls-server-verify. Do not enable this except to use in the unit test. Backport-PR-URL: #19638 Fixes: #1461 PR-URL: #1836 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

In openssl s_client on Windows, RAND_screen() is invoked to initialize random state but it takes several seconds in each connection. This added -no_rand_screen to openssl s_client on Windows to skip RAND_screen() and gets a better performance in the unit test of test-tls-server-verify. Do not enable this except to use in the unit test. Fixes: nodejs#1461 PR-URL: nodejs#1836 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

In openssl s_client on Windows, RAND_screen() is invoked to initialize random state but it takes several seconds in each connection. This added -no_rand_screen to openssl s_client on Windows to skip RAND_screen() and gets a better performance in the unit test of test-tls-server-verify. Do not enable this except to use in the unit test. Fixes: #1461 PR-URL: #1836 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

In openssl s_client on Windows, RAND_screen() is invoked to initialize random state but it takes several seconds in each connection. This added -no_rand_screen to openssl s_client on Windows to skip RAND_screen() and gets a better performance in the unit test of test-tls-server-verify. Do not enable this except to use in the unit test. Fixes: nodejs#1461 PR-URL: nodejs#1836 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

In openssl s_client on Windows, RAND_screen() is invoked to initialize random state but it takes several seconds in each connection. This added -no_rand_screen to openssl s_client on Windows to skip RAND_screen() and gets a better performance in the unit test of test-tls-server-verify. Do not enable this except to use in the unit test. Fixes: #1461 Backport-PR-URL: #28230 PR-URL: #1836 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

In openssl s_client on Windows, RAND_screen() is invoked to initialize random state but it takes several seconds in each connection. This added -no_rand_screen to openssl s_client on Windows to skip RAND_screen() and gets a better performance in the unit test of test-tls-server-verify. Do not enable this except to use in the unit test. Fixes: nodejs/node#1461 PR-URL: nodejs/node#1836 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

orangemocha mentioned this pull request May 29, 2015

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

Closed

Fishrock123 added windows Issues and PRs related to the Windows platform. test Issues and PRs related to the tests. openssl Issues and PRs related to the OpenSSL dependency. labels May 29, 2015

bnoordhuis reviewed May 30, 2015
View reviewed changes

joaocgreis and others added 6 commits June 1, 2015 18:27

test: run tls-server-verify servers in parallel

1bcb5f0

Different servers must use different ports. Since we can count only on common.PORT and common.PORT+1, run only 2 servers in parallel. (cherry picked from commit efb0075ee0e1f4123974fe5b74ded8727bad4a9e)

test: improve console output of tls-server-verify

f49019f

When running in parallel, it is not easy to identify what server and client failed when the test fails. This adds identifiers to all lines of console output. (cherry picked from commit 29c651522df2f47c4360660264eb857fb6232b23)

test: kill child in tls-server-verify for speed up

0368003

For better performance of the test, the parent kills child processes so as not to wait them to be ended. (cherry picked from commit 833b23636045f7afc929196139021630a390391a)

test: add -no_rand_screen for tls-server-verify

e809f01

This improves the performance of openssl s_client on Windows and gains several seconds to finish test-tls-server-verify. (cherry picked from commit 2ff517e0e410ea33ba5a3d289a82fc315d120e8e)

joaocgreis force-pushed the joaocgreis-io-tls-server-verify branch from ec14721 to e809f01 Compare June 1, 2015 17:59

joaocgreis mentioned this pull request Jun 2, 2015

test,win: speedup tls-server-verify nodejs/node-v0.x-archive#25368

Closed

bnoordhuis force-pushed the master branch from 311c3e4 to b926718 Compare June 2, 2015 17:25

evanlucas mentioned this pull request Dec 7, 2017

v9.2.1 release proposal #17531

Merged

This was referenced Dec 7, 2017

v8.9.3 proposal #17532

Merged

v6.12.2 proposal #17533

Merged

v4.8.7 proposal #17534

Merged

joyeecheung mentioned this pull request Jan 26, 2018

test: fix test/parallel/test-tls-server-verify.js on Windows CI #18382

Closed

2 tasks

MylesBorins mentioned this pull request Aug 17, 2018

v8.12.0 proposal #21593

Merged

BethGriggs mentioned this pull request Sep 19, 2019

v8.16.2 proposal #29617

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test,win: speedup tls-server-verify #1836

test,win: speedup tls-server-verify #1836

orangemocha commented May 29, 2015

orangemocha commented May 29, 2015

piscisaureus commented May 29, 2015

rvagg commented May 30, 2015

shigeki commented May 30, 2015

mathiask88 commented May 30, 2015

bnoordhuis May 30, 2015

shigeki Jun 1, 2015

shigeki commented Jun 1, 2015

shigeki commented Jun 1, 2015

joaocgreis commented Jun 2, 2015

bnoordhuis commented Jun 2, 2015

orangemocha commented Jun 3, 2015

test,win: speedup tls-server-verify #1836

test,win: speedup tls-server-verify #1836

Conversation

orangemocha commented May 29, 2015

orangemocha commented May 29, 2015

piscisaureus commented May 29, 2015

rvagg commented May 30, 2015

shigeki commented May 30, 2015

mathiask88 commented May 30, 2015

bnoordhuis May 30, 2015

Choose a reason for hiding this comment

shigeki Jun 1, 2015

Choose a reason for hiding this comment

shigeki commented Jun 1, 2015

shigeki commented Jun 1, 2015

joaocgreis commented Jun 2, 2015

bnoordhuis commented Jun 2, 2015

orangemocha commented Jun 3, 2015