Feature/tls wrap #5645

indutny · 2013-06-08T16:24:00Z

WIP, doesn't work with concurrent connections (for some yet unknown reason).

Usage example: https://gist.github.com/indutny/f60d9724b0513de242ec

Nodejs-Jenkins · 2013-06-08T16:24:05Z

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit indutny/node@304a0a3 has the following error(s):

Commit message must indicate the subsystem this commit changes

Commit indutny/node@67be8ed has the following error(s):

Commit message must indicate the subsystem this commit changes

Commit indutny/node@2f3c21b has the following error(s):

Commit message must indicate the subsystem this commit changes

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

indutny · 2013-06-08T17:01:04Z

Ok, fixed parallel requests error.

indutny · 2013-06-08T17:04:52Z

Surprisingly it seems to be almost 2x faster than tls module on my MBP: 840 rps vs 449 rps.

NOTE: comparing this scripts https://gist.github.com/indutny/b9913c3e2c99a20bbece

bnoordhuis · 2013-06-08T20:58:39Z

src/node_crypto_bio.h

s/amout/amount/

bnoordhuis · 2013-06-08T21:18:10Z

I like where this is going, Fedor.

indutny · 2013-06-09T14:52:19Z

New API - new benchmark: https://gist.github.com/69ea2768bad63f65babb.git

@bnoordhuis @tjfontaine thoughts?

bnoordhuis · 2013-06-11T11:38:43Z

src/node_wrap.h

What's up with style?

bnoordhuis · 2013-06-11T22:49:40Z

src/tls_wrap.cc

This is probably not your code but casting away return values is so very '80s.

indutny · 2013-06-14T10:08:13Z

Whoa, this took really long time to get this working properly... I invite you (@bnoordhuis and @isaacs, probably others) to review it once again before merging this into master!

indutny · 2013-06-14T10:27:51Z

And here are promising benchmark results (4 runs on osx):

tls/throughput.js dur=5 type=buf size=2: ./node-tls-wrap: 1.2469 ./node: 0.77893 ...... 60.07%
tls/throughput.js dur=5 type=buf size=1024: ./node-tls-wrap: 256.62 ./node: 189.25 .... 35.60%
tls/throughput.js dur=5 type=buf size=1048576: ./node-tls-wrap: 316.12 ./node: 329.04 . -3.93%
tls/throughput.js dur=5 type=asc size=2: ./node-tls-wrap: 1.2086 ./node: 0.67809 ...... 78.24%
tls/throughput.js dur=5 type=asc size=1024: ./node-tls-wrap: 240.74 ./node: 161.87 .... 48.73%
tls/throughput.js dur=5 type=asc size=1048576: ./node-tls-wrap: 306.87 ./node: 302.1 ... 1.58%
tls/throughput.js dur=5 type=utf size=2: ./node-tls-wrap: 1.2178 ./node: 0.66881 ...... 82.08%
tls/throughput.js dur=5 type=utf size=1024: ./node-tls-wrap: 249.63 ./node: 154.41 .... 61.66%
tls/throughput.js dur=5 type=utf size=1048576: ./node-tls-wrap: 288.85 ./node: 290.64 . -0.62%
tls/tls-connect.js concurrency=1 dur=5: ./node-tls-wrap: 434.57 ./node: 282.75 ........ 53.70%
tls/tls-connect.js concurrency=10 dur=5: ./node-tls-wrap: 467.16 ./node: 304.56 ....... 53.39%

Allocate buffer only if the next one isn't free.

bnoordhuis · 2013-06-15T19:11:21Z

lib/_tls_wrap.js

Merge with regexpEscape() from lib/repl.js? You probably want to move it to lib/util.js.

Right, will do it after merge in a separate commit.

@tjfontaine

Originally contributed by @tjfontaine, but modified to be faster and more generic.

StreamWrapCallbacks is a helper class for incepting into uv_stream_t* management process.

Split `tls.js` into `_tls_legacy.js`, containing legacy `createSecurePair` API, and `_tls_wrap.js` containing new code based on `tls_wrap` binding. Remove tests that are no longer useful/valid.

bnoordhuis · 2013-06-15T20:15:51Z

src/tls_wrap.cc

0 or 1? That rather subverts the meaning of the .writeQueueSize property, doesn't it?

I think I will just set it to 1 in javascript, its needed to make code work with net.js.

Then .writeQueueSize should be changed to something like .hasPendingWrites=true|false.

bnoordhuis · 2013-06-15T21:01:38Z

A few more comments but mostly LGTM.

isaacs · 2013-06-17T01:25:32Z

I like how the API is a lot simpler for users to interact with, and most of the code looks good. But we have to figure out sessions before this is shippable, I think.

indutny · 2013-06-17T10:17:41Z

Thanks, landed in dc50f27, af80e7b, 03e008d, 4c48a39, 6978e99, 0495b70, 5dd155a, 4536b27 and 56d9c48.

bnoordhuis reviewed Jun 8, 2013
View reviewed changes

src/node_crypto_bio.h Outdated

Copy link

Member

bnoordhuis Jun 8, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/amout/amount/

indutny mentioned this pull request Jun 11, 2013

tls on connection memory leak #5520

Closed

bnoordhuis reviewed Jun 11, 2013
View reviewed changes

src/tls_wrap.cc

Copy link

Member

bnoordhuis Jun 11, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably not your code but casting away return values is so very '80s.

indutny added 2 commits June 14, 2013 17:10

crypto: fix excessive buffer allocation

56d9c48

Allocate buffer only if the next one isn't free.

crypto: do not deallocate embedded buffer

4536b27

bnoordhuis reviewed Jun 15, 2013
View reviewed changes

indutny added 7 commits June 15, 2013 21:43

crypto: ensure that read head is always non-empty

5dd155a

crypto: clear error on return in AddCRL

0495b70

process: abstract out HandleToStream

6978e99

Originally contributed by @tjfontaine, but modified to be faster and more generic.

stream_wrap: introduce StreamWrapCallbacks

4c48a39

StreamWrapCallbacks is a helper class for incepting into uv_stream_t* management process.

tls_wrap: embed TLS encryption into streamwrap

c064da4

tls: introduce TLSSocket based on tls_wrap binding

5a22cce

Split `tls.js` into `_tls_legacy.js`, containing legacy `createSecurePair` API, and `_tls_wrap.js` containing new code based on `tls_wrap` binding. Remove tests that are no longer useful/valid.

tls: share socket._hadError with http_client

c592d87

bnoordhuis reviewed Jun 15, 2013
View reviewed changes

indutny closed this Jun 17, 2013

indutny deleted the feature/tls-wrap branch June 17, 2013 10:17

Uh oh!

Feature/tls wrap #5645

Feature/tls wrap #5645

Uh oh!

Conversation

indutny commented Jun 8, 2013

Uh oh!

Nodejs-Jenkins commented Jun 8, 2013

Uh oh!

indutny commented Jun 8, 2013

Uh oh!

indutny commented Jun 8, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jun 8, 2013

Uh oh!

indutny commented Jun 9, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indutny commented Jun 14, 2013

Uh oh!

indutny commented Jun 14, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jun 15, 2013

Uh oh!

isaacs commented Jun 17, 2013

Uh oh!

indutny commented Jun 17, 2013

Uh oh!

Uh oh!