crypto: add sync methods, deprecate optional callbacks #632

jonathanong · 2015-01-28T03:54:04Z

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?

charmander · 2015-01-28T04:01:58Z

lib/crypto.js


-exports.rng = exports.prng = randomBytes;
+exports.rng = util.deprecateSync(randomBytes, '`crypto.rng(size)` is deprecated. Please us `crypto.rngSync(size)` instead.');


“Please us” → “Please use”

brendanashworth · 2015-02-01T07:04:37Z

lib/util.js

+// except it only warns if the last argument is not a callback function.
+exports.deprecateSync = function(fn, msg) {
+  // Allow for deprecating things in the process of starting up.
+  if (isUndefined(global.process)) {


Switch away from isUndefined please, see 6ac8bcd.

jonathanong · 2015-02-02T00:45:56Z

removed all those stupid utilities :D

vkurchatkin · 2015-02-02T07:33:57Z

lib/util.js

+// Mark that the synchronous version of a function is deprecated.
+// This is essentially the same as `exports.deprecate()`,
+// except it only warns if the last argument is not a callback function.
+exports.deprecateSync = function(fn, msg) {


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.

bnoordhuis · 2015-02-02T15:22:41Z

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

jonathanong · 2015-02-02T21:02:34Z

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

vkurchatkin · 2015-02-02T21:04:12Z

@jonathanong run make lint

jonathanong · 2015-02-02T21:34:20Z

lines too long :( will fix later

jonathanong · 2015-02-03T02:39:41Z

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

brendanashworth · 2015-02-10T08:38:46Z

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.

jonathanong · 2015-02-17T02:59:45Z

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?

brendanashworth · 2015-03-05T07:58:48Z

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.

jonathanong · 2015-03-08T05:32:38Z

@brendanashworth ping

brendanashworth · 2015-03-08T06:00:25Z

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?

jonathanong · 2015-03-08T22:47:54Z

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

brendanashworth · 2015-04-05T21:09:05Z

doc/api/crypto.markdown

+may conceivably block is right after boot, when the whole system is still
+low on entropy.
+
+NOTE: The synchronous API for this method is deprecated - please use `crypto.randomBytesSync(size)` instead.


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

brendanashworth · 2015-04-05T21:22:38Z

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

indutny · 2015-04-06T09:34:11Z

doc/api/crypto.markdown

    crypto.randomBytes(256, function(ex, buf) {
      if (ex) throw ex;
      console.log('Have %d bytes of random data: %s', buf.length, buf);
    });

-    // sync
+NOTE: This will block if there is insufficient entropy, although it should


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.

indutny · 2015-04-06T09:35:56Z

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)?

jonathanong · 2015-04-06T19:16:34Z

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

brendanashworth · 2015-04-24T05:56:59Z

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)).

brendanashworth · 2015-05-06T04:26:24Z

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

jonathanong · 2015-05-06T07:15:49Z

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

bnoordhuis · 2015-05-06T11:59:44Z

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

thefourtheye · 2015-07-09T22:38:08Z

doc/api/crypto.markdown

+
+## crypto.randomBytesSync(size)
+
+A synchronous version of `crypto.randomBytes(size)`. Usage:


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

thefourtheye · 2015-07-09T22:49:45Z

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>

Trott · 2015-07-10T05:19:10Z

@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>

thefourtheye · 2015-07-12T15:41:30Z

lib/crypto.js

+    '`crypto.pseudoRandomBytes()` is deprecated. ' +
+    'Please use `crypto.randomBytes()` instead.');
+
+exports.rng = util._deprecateSync(randomBytes,


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.

silverwind · 2015-07-12T20:18:38Z

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

jonathanong · 2015-07-12T20:27:45Z

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

jonathanong · 2015-07-12T20:28:58Z

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

silverwind · 2015-07-12T20:29:24Z

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

jonathanong · 2015-07-12T20:30:34Z

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

Fishrock123 · 2015-07-12T20:30:58Z

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.

jonathanong · 2015-07-12T20:31:53Z

#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>

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


		exports.rng = exports.prng = randomBytes;
		exports.rng = util.deprecateSync(randomBytes, '`crypto.rng(size)` is deprecated. Please us `crypto.rngSync(size)` instead.');


		## crypto.randomBytesSync(size)

		A synchronous version of `crypto.randomBytes(size)`. Usage:

crypto: add sync methods, deprecate optional callbacks #632

crypto: add sync methods, deprecate optional callbacks #632

Conversation

jonathanong commented Jan 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanong commented Feb 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Feb 2, 2015

jonathanong commented Feb 2, 2015

vkurchatkin commented Feb 2, 2015

jonathanong commented Feb 2, 2015

jonathanong commented Feb 3, 2015

brendanashworth commented Feb 10, 2015

jonathanong commented Feb 17, 2015

brendanashworth commented Mar 5, 2015

jonathanong commented Mar 8, 2015

brendanashworth commented Mar 8, 2015

jonathanong commented Mar 8, 2015

Choose a reason for hiding this comment

brendanashworth commented Apr 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Apr 6, 2015

jonathanong commented Apr 6, 2015

brendanashworth commented Apr 24, 2015

brendanashworth commented May 6, 2015

jonathanong commented May 6, 2015

bnoordhuis commented May 6, 2015

Choose a reason for hiding this comment

thefourtheye commented Jul 9, 2015

Trott commented Jul 10, 2015

Choose a reason for hiding this comment

silverwind commented Jul 12, 2015

jonathanong commented Jul 12, 2015

jonathanong commented Jul 12, 2015

silverwind commented Jul 12, 2015

jonathanong commented Jul 12, 2015

Fishrock123 commented Jul 12, 2015

jonathanong commented Jul 12, 2015