crypto: add sync methods, deprecate optional callbacks by jonathanong · Pull Request #632 · nodejs/node · GitHub

for #5. rebased and reopened from #189 (because i forgot about this PR when deleting my fork...).

deprecated optional callbacks for .randomBytes()
deprecated .pseudoRandomBytes() as now it's just an alias for .randomBytes(). another option is to ignore any deprecation messages for pseudo*, but I'm not adding a *Sync for method for them.
added a .deprecateSync() utility

TODO:

docs

Questions:

would you like me to add/change tests?
didn't bother disallowing callbacks in the sync version. after all this is about the public API. is that something you guys want?
want doc updates?
only changed .randomBytes() and .pseudoRandomBytes(). were there others?

“Please us” → “Please use”

Switch away from isUndefined please, see 6ac8bcd.

removed all those stupid utilities :D

make it _private

wouldn't that mean deprecate() should be private too?

the idea is that if we don't have intention of making function public (and documenting it), it should be marked as private. Since it's used only in crypto I'm not even sure there is a point in defining it in utils.

@jonathanong Can you run make test? I imagine a number of tests need updating and I'm sure the linter will croak.

seems fine to me:

Jonathans-MacBook-Pro:io.js jong$ make test
/Applications/Xcode.app/Contents/Developer/usr/bin/make -C out BUILDTYPE=Release V=1
make[1]: Nothing to be done for `all'.
ln -fs out/Release/iojs iojs
/usr/local/opt/python/bin/python2.7 tools/test.py --mode=release message parallel sequential -J
=== release test-dns-cares-domains ===                                         
Path: parallel/test-dns-cares-domains
Command: out/Release/iojs /Users/jong/Workspace/io.js/test/parallel/test-dns-cares-domains.js
--- TIMEOUT ---
[01:08|% 100|+ 793|-   1]: Done                            
make: *** [test] Error 1

@jonathanong run make lint

lines too long :( will fix later

okay it asks for 4 space indents, which is different than other stuff. pretty lost. lint passes though.

I believe a test update would be necessary here, I have no idea what is happening here though.

I'd say that we should disallow the use of the new Sync() APIs with callbacks because it doesn't follow how the rest of io.js handles synchronousness.

to be fair, i don't really want to edit any current tests. i only want to make new ones.

i'm okay with disallowing the use of callbacks with the Sync() APIs, but i'm not sure if it's actually required or useful in any way. if it's a new API and not a documented, i'm not sure how people would even try to use the Sync() API with a callback.

what would you like to test specifically? whether the deprecation messages gets logged? the sync APIs?

I understand about editing current tests; I've seen plenty of deprecated stuff in the tests anyways.

I guess that makes sense too about the Sync() calls - it might be too much of a hassle for something probably nobody will ever do. I'd just like tests that use the new methods to make sure they are called properly and some future PR botches the whole functionality without any tests failing because they all used the old, now deprecated function calls.

I'd like to get this in before 2.0.0 (#1061) btw so that we could potentially remove the old functionality in 3.0.0.

@brendanashworth ping

besides my comments, I think the only thing necessary now would be the doc updates, and even then, we just need crypto.randomBytes updated, as seemingly .rng and .prng aren't documented?

oops. i forgot to commit the super simple test i added. also, added docs. amend and squash at will!

This line is over 80 characters, could you wrap it?

Minus the comment, this LGTM now. @iojs/crypto?

Are you sure about this? ;)

no. i didn't change any of the copy. it just shows up in the diff because code moved around.

I'm fine with the change, but it is rather questionable to me if we need the async version in most of the cases. Could anyone please do the benchmarks for a small amount of data (like 64-128 bytes)?

would be great if there's no need for the async version!

Using this benchmark, macbook pro ~2013:

diff --git a/benchmark/crypto/random-bytes.js b/benchmark/crypto/random-bytes.js
new file mode 100644
index 0000000..452cd8a
--- /dev/null
+++ b/benchmark/crypto/random-bytes.js
@@ -0,0 +1,19 @@
+var common = require('../common');
+var randomBytes = require('crypto').randomBytes;
+var bench = common.createBenchmark(main, {
+  n: [1e5],
+  len: [32, 64, 128, 256, 1024]
+});
+
+function main(conf) {
+  var len = conf.len | 0;
+  var n = conf.n | 0;
+
+  bench.start();
+
+  for (var i = 0; i < n; i++) {
+    randomBytes(len);
+  }
+
+  bench.end(n);
+}

I'm getting numbers like this:

$ iojs -v
v2.0.0
$ iojs benchmark/crypto/random-bytes
crypto/random-bytes.js n=100000 len=32: 268588.46801
crypto/random-bytes.js n=100000 len=64: 225115.42416
crypto/random-bytes.js n=100000 len=128: 186871.09572
crypto/random-bytes.js n=100000 len=256: 124182.33831
crypto/random-bytes.js n=100000 len=1024: 41259.32223

As per request [here](nodejs#632 (comment)).

tl;dr: async randomBytes is nowhere near necessary on a macbook pro, probably unnecessary for anything but integrated hardware

i fine with deprecating all the "random" APIs in favor of just a synchronous .randomBytes()!

i fine with deprecating all the "random" APIs in favor of just a synchronous .randomBytes()!

See this code comment: randomness is usually but not always immediate.

A secondary issue is that it's computationally expensive; generating a lot of randomness, even when there is enough entropy, will block the event loop for some time.

Semi-related: nodejs/node-v0.x-archive#7338

It kinda gives me a feeling that there are more than one synchronous implementations and this is one of them.

Hmmm, can we defer merging this for some more time, if that's okay? I would like to properly go through the util changes (Its really difficult to do it in Mobile)

Verify that passing a non-number will throw and that the argument is returned on success. PR-URL: #2121 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

On windows, there's no need to continue with the msbuild process (signing, whatnot) when we only want to clean the project. PR-URL: #2127 Reviewed-By: Alexis Campailla <alexis@janeasystems.com>

@thefourtheye Sure, it can wait a little bit.

If the proxy objects don't have a valid `hasOwnPropertyNames` trap, REPL crashes with a `TypeError`, as per the bug report #2119 > var proxy = Proxy.create({ fix: function() { return {}; } }); undefined > proxy.<tab> TypeError: Proxy handler #<Object> has no 'getOwnPropertyNames' trap at Function.getOwnPropertyNames (native) at repl.js:644:40 at REPLServer.defaultEval (repl.js:169:5) at bound (domain.js:254:14) at REPLServer.runBound [as eval] (domain.js:267:12) at REPLServer.complete (repl.js:639:14) at REPLServer.complete [as completer] (repl.js:207:10) at REPLServer.Interface._tabComplete (readline.js:377:8) at REPLServer.Interface._ttyWrite (readline.js:845:14) at ReadStream.onkeypress (readline.js:105:10) This patch traps the error thrown and suppresses it. PR-URL: #2120 Fixes: #2119 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

configure was getting called twice. We also erroneously introduced support for VS2010, and were picking 2010 before other versions. PR-URL: #2131 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Adding a single rule to be called from Jenkins. Jenkins jobs typically call: python ./configure make -j $(getconf _NPROCESSORS_ONLN) make test-ci After this change, we can have Jenkins call: make run-ci -j $(getconf _NPROCESSORS_ONLN) This allows us to customize how we call configure for different repos or branches (e.g. joyent\node). PR-URL: #2134 Reviewed-By: Ryan Graham <r.m.graham@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

When the listener was truthy but NOT a function, fs.watchFile would throw an error through the EventEmitter. This caused a problem because it would only be thrown after the listener was started, which left the listener on. There should be no backwards compatability issues because the error was always thrown, just in a different manner. Also adds tests for this and other basic functionality. PR-URL: #2093 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

When fs.watchFile encounters an ENOENT error, it invokes the given callback with some error data. This caused an issue as it was different behaviour than Node v0.10. Instead of changing this behaviour, document it and add a test. Ref: #1745 Ref: #2028 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> PR-URL: #2093

- This makes v8 add .trace_function_info to the serialized form of snapshots from v8::HeapSnapshot::Serialize - .trace_funciton_info combined with .trace_node in snapshots tells the JS location that allocated a specific object PR-URL: #2135 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

`emitKeys` is a generator which emits `keypress` events in an infinite loop. But if `keypress` event handler throws, the error stops the loop, leaving generator in a broken state. So this patch restarts the generator when an error occures. PR-URL: #2107 Reviewed-By: Christopher Monsanto <chris@monsan.to> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Re-use `assertPath()` when asserting path argument types in `join()` as throughout the rest of the `path` module. This also ensures the same error message generated for posix as for win32. PR-URL: #2159 Reviewed-By: Roman Reiss <me@silverwind.io>

A persistent failure on OS X 10.11 uncovered a inproperly cleaned up temp directory in this test. This changes the mkdirSync call to clean up properly in case it throws. PR-URL: #2164 Reviewed-By: Evan Lucas <evanlucas@me.com>

Fixes the arguments comments for execFileSync and other related minor inconsistencies in commented arguments in the same file. PR-URL: #2161 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Instead of introducing a new deprecation method, you can simply do do the callback check and deprecation message in node_crypto.cc' RandomBytes function. @silverwind, please correct me if I am wrong.

@jonathanong if you could rebase your branch against current master, I'll provide a .patch for you to fix the deprecation.

oh shit. this is a PR against v1.x. i rebased it against master. now the diff looks terrible.

what do you want to do? cherry-pick the commits? open a new PR against master (or next or whichever branch)?

Did you miss a -f? Otherwise I'd just open a new PR with a reference to this one.

@silverwind no it's just against the wrong branch. i think i opened this before you guys moved to master or something

You may be able to fix it with https://github.com/github/hub#git-pull-request monkeying, but it's easier to just open a new PR and reference it back.

#2170

charmander reviewed Jan 28, 2015
View reviewed changes

brendanashworth added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 28, 2015

brendanashworth reviewed Feb 1, 2015
View reviewed changes

vkurchatkin reviewed Feb 2, 2015
View reviewed changes

mscdex added the crypto Issues and PRs related to the crypto subsystem. label Mar 22, 2015

brendanashworth reviewed Apr 5, 2015
View reviewed changes

indutny reviewed Apr 6, 2015
View reviewed changes

chrisdickinson added the land-on-master label Apr 17, 2015

brendanashworth added a commit to brendanashworth/io.js that referenced this pull request Apr 24, 2015

benchmark: add crypto.randomBytes()

e71875b

As per request [here](nodejs#632 (comment)).

thefourtheye reviewed Jul 9, 2015
View reviewed changes

evanlucas and others added 2 commits July 9, 2015 23:23

test: add test for dgram.setTTL

842eb5b

Verify that passing a non-number will throw and that the argument is returned on success. PR-URL: #2121 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

build: bail early if clean is invoked

47e2c5c

On windows, there's no need to continue with the msbuild process (signing, whatnot) when we only want to clean the project. PR-URL: #2127 Reviewed-By: Alexis Campailla <alexis@janeasystems.com>

thefourtheye and others added 10 commits July 10, 2015 07:54

build: fix vcbuild merge issues

84012c9

configure was getting called twice. We also erroneously introduced support for VS2010, and were picking 2010 before other versions. PR-URL: #2131 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

path: assert path.join() arguments equally

2ba8460

Re-use `assertPath()` when asserting path argument types in `join()` as throughout the rest of the `path` module. This also ensures the same error message generated for posix as for win32. PR-URL: #2159 Reviewed-By: Roman Reiss <me@silverwind.io>

test: properly clean up temp directory

d4ceb16

A persistent failure on OS X 10.11 uncovered a inproperly cleaned up temp directory in this test. This changes the mkdirSync call to clean up properly in case it throws. PR-URL: #2164 Reviewed-By: Evan Lucas <evanlucas@me.com>

child_process: fix arguments comments

5acad6b

Fixes the arguments comments for execFileSync and other related minor inconsistencies in commented arguments in the same file. PR-URL: #2161 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

thefourtheye reviewed Jul 12, 2015
View reviewed changes

jonathanong added 3 commits July 12, 2015 13:26

crypto: add sync methods, deprecate optional callbacks

974a1b8

test: add simple test for crypto.randomBytesSync()

dbf3519

doc: add crypto.randomBytesSync(size)

91a357b

jonathanong closed this Jul 12, 2015

jonathanong mentioned this pull request Jul 12, 2015

Deprecate mixed async crypto #2170

Closed

crypto: add sync methods, deprecate optional callbacks by jonathanong · Pull Request #632 · nodejs/node · GitHub

Repository navigation

crypto: add sync methods, deprecate optional callbacks - #632

crypto: add sync methods, deprecate optional callbacks#632jonathanong wants to merge 403 commits intonodejs:v1.xnodejs/node:v1.xfrom jonathanong:deprecate-mixed-async-cryptoCopy head branch name to clipboard

Conversation

jonathanong commented Jan 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanong commented Feb 2, 2015

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 Feb 2, 2015

Uh oh!

jonathanong commented Feb 2, 2015

Uh oh!

vkurchatkin commented Feb 2, 2015

Uh oh!

jonathanong commented Feb 2, 2015

Uh oh!

jonathanong commented Feb 3, 2015

Uh oh!

brendanashworth commented Feb 10, 2015

Uh oh!

jonathanong commented Feb 17, 2015

Uh oh!

brendanashworth commented Mar 5, 2015

Uh oh!

jonathanong commented Mar 8, 2015

Uh oh!

brendanashworth commented Mar 8, 2015

Uh oh!

jonathanong commented Mar 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendanashworth commented Apr 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indutny commented Apr 6, 2015

Uh oh!

jonathanong commented Apr 6, 2015

Uh oh!

brendanashworth commented Apr 24, 2015

Uh oh!

brendanashworth commented May 6, 2015

Uh oh!

jonathanong commented May 6, 2015

Uh oh!

bnoordhuis commented May 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Jul 9, 2015

Uh oh!

Trott commented Jul 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind commented Jul 12, 2015

Uh oh!

jonathanong commented Jul 12, 2015

Uh oh!

jonathanong commented Jul 12, 2015

Uh oh!

silverwind commented Jul 12, 2015

Uh oh!

jonathanong commented Jul 12, 2015

Uh oh!

Fishrock123 commented Jul 12, 2015

crypto: add sync methods, deprecate optional callbacks#632
jonathanong wants to merge 403 commits into
nodejs:v1.xfrom
jonathanong:deprecate-mixed-async-crypto