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

orangemocha · 2015-05-22T16:12:20Z

Addresses #25279 (and the related nodejs/node#1461 once ported there).

The test now takes half as long as it did before.

orangemocha · 2015-05-22T16:17:45Z

@indutny , I noticed that in the test case that has renegotiate:true only connection with the first client is renegotiated, and the test depends on that. That's why we didn't parallelize clients if renegotiate is true. Was that intentional / mean to test that specific sequence? If not, we could refactor that test case to speed up things a bit more.

orangemocha · 2015-05-28T16:00:18Z

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.

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.

Different servers must use different ports. Since we can count only on common.PORT and common.PORT+1, run only 2 servers in parallel.

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.

Now that the test is fixed, node-accept-pull-request should fail when the test fails.

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

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 9f0f7c3)

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

joaocgreis · 2015-06-02T10:47:35Z

I updated the commits as suggested in nodejs/node#1836 .

orangemocha · 2015-06-03T18:09:06Z

@joyent/node-coreteam : these changes already landed in io.js with nodejs/node#1836. Can you give a quick +1 so I can land this PR here?

Please note that the commits about -no_rand_screen are floated on top of openssl.

jasnell · 2015-06-03T18:13:48Z

LGTM

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. Reviewed-By: James M Snell <[email protected]> PR-URL: #25368

Different servers must use different ports. Since we can count only on common.PORT and common.PORT+1, run only 2 servers in parallel. Reviewed-By: James M Snell <[email protected]> PR-URL: #25368

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. Reviewed-By: James M Snell <[email protected]> PR-URL: #25368

Now that the test is fixed, node-accept-pull-request should fail when the test fails. Reviewed-By: James M Snell <[email protected]> PR-URL: #25368

For better performance of the test, the parent kills child processes so as not to wait them to be ended. (cherry picked from commit 833b236) Reviewed-By: James M Snell <[email protected]> PR-URL: #25368

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 9f0f7c3) Reviewed-By: James M Snell <[email protected]> PR-URL: #25368

This improves the performance of openssl s_client on Windows and gains several seconds to finish test-tls-server-verify. (cherry picked from commit 2ff517e) Reviewed-By: James M Snell <[email protected]> PR-URL: #25368

orangemocha · 2015-06-04T13:32:20Z

Thank you, landed in 1034982 and adjacent commits.

misterdjules · 2015-06-04T18:20:43Z

@orangemocha Thanks! Could you please add the floating patches to the list here: https://github.com/joyent/node/wiki/OpenSSL-upgrade-process#floating-patches? Also, is there a chance that these patches are landed in OpenSSL upstream?

orangemocha · 2015-06-05T12:08:39Z

Could you please add the floating patches to the list here: https://github.com/joyent/node/wiki/OpenSSL-upgrade-process#floating-patches?

Updated. Thanks for reminding me @misterdjules !

Also, is there a chance that these patches are landed in OpenSSL upstream?

Quoting @shigeki :

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.

misterdjules · 2015-06-05T22:16:33Z

@orangemocha @shigeki Thank you and great work on that one again! Out of curiosity, have we tried to actually upstream the s_client patch? Maybe a compromise could be reached that would avoid floating another patch?

shigeki · 2015-06-08T16:49:09Z

@misterdjules I think that this patch is hard to be accepted to the upstream. The code of RAND_screen is really old code since SSLeay about more than 10 years ago and I cannot insist my confidence that we have enough entropy to be secure without this in a general use case. As for floating patch to openssl-clie.exe in node repo for using unit test, another fix of keypress have already been introduced in https://github.com/joyent/node/commits/master/deps/openssl/openssl/apps/s_client.c . We have two floating patches as a result.

misterdjules · 2015-06-08T22:26:48Z

@shigeki I put up a list of current floating patches on top of OpenSSL in joyent/node, and we are already at 5 floating patches. I wouldn't be surprised if I missed a few. My main concern is having this list grow over time to something that becomes difficult to manage.

If it doesn't take a lot of effort for you (and if you have time) to send a patch upstream and get some feedback from the OpenSSL project, I would still think that's worth it just to try and see how that goes.

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 9f0f7c3) Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs#25368

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) Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs/node-v0.x-archive#25368

This replaces all sources of openssl-1.0.1r.tar.gz into deps/openssl/openssl PR-URL: #4967 Reviewed-By: Ben Noordhuis <[email protected]> deps: copy all openssl header files to include dir All symlink files in `deps/openssl/openssl/include/openssl/` are removed and replaced with real header files to avoid issues on Windows. Two files of opensslconf.h in crypto and include dir are replaced to refer config/opensslconf.h. PR-URL: #4967 Reviewed-By: Ben Noordhuis <[email protected]> deps: separate sha256/sha512-x86_64.pl for openssl sha256-x86_64.pl does not exist in the origin openssl distribution. It was copied from sha512-x86_64.pl and both sha256/sha512 scripts were modified so as to generates only one asm file specified as its key hash length. PR: #9451 PR-URL: nodejs/node-v0.x-archive#9451 Reviewed-By: Julien Gilli <[email protected]> deps: fix openssl assembly error on ia32 win32 `x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and perhaps others) are requiring .686 . PR: #9451 PR-URL: nodejs/node-v0.x-archive#9451 Reviewed-By: Julien Gilli <[email protected]> openssl: fix keypress requirement in apps on win32 reapply b910613 PR: #9451 PR-URL: nodejs/node-v0.x-archive#9451 Reviewed-By: Julien Gilli <[email protected]> deps: add -no_rand_screen to openssl s_client 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) Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs/node-v0.x-archive#25368

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) Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs/node-v0.x-archive#25368

This replaces all sources of openssl-1.0.1r.tar.gz into deps/openssl/openssl PR-URL: nodejs/node#4967 Reviewed-By: Ben Noordhuis <[email protected]> deps: copy all openssl header files to include dir All symlink files in `deps/openssl/openssl/include/openssl/` are removed and replaced with real header files to avoid issues on Windows. Two files of opensslconf.h in crypto and include dir are replaced to refer config/opensslconf.h. PR-URL: nodejs/node#4967 Reviewed-By: Ben Noordhuis <[email protected]> deps: separate sha256/sha512-x86_64.pl for openssl sha256-x86_64.pl does not exist in the origin openssl distribution. It was copied from sha512-x86_64.pl and both sha256/sha512 scripts were modified so as to generates only one asm file specified as its key hash length. PR: nodejs#9451 PR-URL: nodejs#9451 Reviewed-By: Julien Gilli <[email protected]> deps: fix openssl assembly error on ia32 win32 `x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and perhaps others) are requiring .686 . PR: nodejs#9451 PR-URL: nodejs#9451 Reviewed-By: Julien Gilli <[email protected]> openssl: fix keypress requirement in apps on win32 reapply b910613 PR: nodejs#9451 PR-URL: nodejs#9451 Reviewed-By: Julien Gilli <[email protected]> deps: add -no_rand_screen to openssl s_client 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 9f0f7c3) Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs#25368

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 9f0f7c3) Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs#25368

Nodejs-Jenkins added pr v0.12 labels May 22, 2015

orangemocha added the in progress label May 22, 2015

This was referenced May 22, 2015

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

Closed

test/simple/test-tls-server-verify.js times out on our Windows 2012 Jenkins agent #25279

Closed

joaocgreis and others added 7 commits June 1, 2015 19:03

test: run tls-server-verify servers in parallel

28a6a19

Different servers must use different ports. Since we can count only on common.PORT and common.PORT+1, run only 2 servers in parallel.

test: improve console output of tls-server-verify

7e1a291

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.

test,win: re-enable tls-server-verify in CI

d2f778c

Now that the test is fixed, node-accept-pull-request should fail when the test fails.

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

b5ee2f5

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

test: add -no_rand_screen for tls-server-verify

c9718a4

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

orangemocha added a commit that referenced this pull request Jun 4, 2015

test,win: re-enable tls-server-verify in CI

83b7c07

Now that the test is fixed, node-accept-pull-request should fail when the test fails. Reviewed-By: James M Snell <[email protected]> PR-URL: #25368

orangemocha closed this Jun 4, 2015

orangemocha removed the in progress label Jun 4, 2015

misterdjules added this to the 0.12.5 milestone Jun 4, 2015

rvagg mentioned this pull request Feb 8, 2016

Release proposal: v0.12.10 (LTS) nodejs/node#5137

Closed

rvagg mentioned this pull request Feb 8, 2016

Release proposal: v0.10.42 (Maintenance) nodejs/node#5138

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

orangemocha commented May 22, 2015

orangemocha commented May 22, 2015

orangemocha commented May 28, 2015

joaocgreis commented Jun 2, 2015

orangemocha commented Jun 3, 2015

jasnell commented Jun 3, 2015

orangemocha commented Jun 4, 2015

misterdjules commented Jun 4, 2015

orangemocha commented Jun 5, 2015

misterdjules commented Jun 5, 2015

shigeki commented Jun 8, 2015

misterdjules commented Jun 8, 2015

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

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

Conversation

orangemocha commented May 22, 2015

orangemocha commented May 22, 2015

orangemocha commented May 28, 2015

joaocgreis commented Jun 2, 2015

orangemocha commented Jun 3, 2015

jasnell commented Jun 3, 2015

orangemocha commented Jun 4, 2015

misterdjules commented Jun 4, 2015

orangemocha commented Jun 5, 2015

misterdjules commented Jun 5, 2015

shigeki commented Jun 8, 2015

misterdjules commented Jun 8, 2015