[crypto] randomBytes() throws exceptions without calling callback function

[Mickael-van-der-Beek]

From the official documentation, crypto-randomBytes() can be used asynchronously if you pass a callback function like most I/O method of the Node.js core.

The issue is that if randomBytes() fails, it will throw an exception and not call the callback function with the error argument. e.g:

crypto.randomBytes(-1, function (e, buf) {
    if(e) {
        console.log('Error=', e);
    }

    console.log('Buf'=, buf);
});

This will obviously fail because size is set as -1 but it will fail with by throwing an exception and print:

TypeError: Argument #1 must be number > 0
    at Object.<anonymous> (/SOME_PATH/MY_FILE.js:11:8)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:906:3

rather than calling the callback function and handle the error with the console.log('Error=', e);

The similar pseudoRandomBytes() function has the same issue.

Am I doing something wrong here or are both functions purely synchronous ?

This has nothing to do with domains, but I wondered if #5801 fixed this issue too ?

[OrangeDog]

Errors in input, throughout Node, will throw exceptions.
Not validating input is considered a programmer error, not a runtime error.

[Mickael-van-der-Beek]

I understand that the mistake is on the programmer's side, but shouldn't Node.js just pass the error to the callback function so that the application can handle the error smoothly ?

Wrapping every Node.js core module call in domains would be quite tedious.

Most NPM modules don't support domains or at least not correctly so relying on callbacks as the basic error handling / bubbling-up mechanism seems to be a better idea.

Otherwise, a developer would have to guess more or less randomly if he can rely on callbacks or use domains / try-catch blocks instead.

[OrangeDog]

This is discussed over many tickets and blog posts.
Programmer errors should terminate the process ASAP.
Developers should be fixing their errors instead of adding more code to try and handle them.

https://www.joyent.com/developers/node/design/errors

[Mickael-van-der-Beek]

A month or so ago I opened a similar input data issue #8249 where a child process would hang if the args argument was a string rather than an array.

So in this case Node.js doesn't throw an exception and also doesn't return an error to the callback function.

Of course this is a bug and some contributors already worked on this issue but what I mean by this example is that the line between what Node.js should or shouldn't take care of is not always that clear.

In the child process case, Node.js has to validate the input data to fail correctly.

[Mickael-van-der-Beek]

In the C++ bindings, the RandomBytes() function that is used by randomBytes() and pseudoRandomBytes() even tests if the size argument is a UInt32 or not:

https://github.com/joyent/node/blob/master/src/node_crypto.cc#L4725

Which is effectively input validation (of the arguments against a function spec) to throw the error.
Why not call the callback function with an error argument instead ?

[chrisdickinson]

@Mickael-van-der-Beek Sorry about #8249, I have a PR out to fix that and I need to update it (#8277). I also have a PR out that I need to update that adds documentation about errors in Node (#8114) that should clarify a bit.

The idea is that, if a program is providing invalid parameters to Node, that program is invalid and should terminate as early as possible. It should not be possible to conflate "programmer"[1] errors (things in the program which are guaranteed not to work; for example, providing a Node API incorrect parameters) with "operational" errors. Operational errors occur during the lifetime of the process -- out of memory, files not existing when they should, files existing when they shouldn't, the network being down, input being invalid, etc. Those are the errors programs should be written to handle, via the appropriate method -- be that callbacks, stream/event emitter error handlers, promises (if you so choose) etc.

Programs run in two phases -- while they're being developed, and while they're in production. The two phases have very different goals, with regards to error handling. In development, exceptions are unexceptional. They are a form of feedback, telling the programmer that they need to revisit the program text and make modifications. As such, it's important for Node to make that feedback available as immediately as possible. Deferring those errors to a callback delays the feedback and conflates programmer errors with operational errors.

In production, exceptions should be truly exceptional, and are feedback that the program needs to be rolled back to an earlier version, which is a expensive operation compared to responding to errors in development (in terms of human adrenaline spent). In practice, if "programmer" errors are treated the same as "operational" errors, they tend to slip into production environments: code that's guarding for operational errors may erroneously handle programmer errors. If this happens, there's a non-zero potential for those errors to compound into a harder-to-debug error.

A good example of "programmer" errors compounding into a harder to debug error: "undefined is not a function". I've seen (and personally written) code that creates this error that has made it to production environments. The problem is that, somewhere in the lifetime of the callee side of the call expression, the value was assigned to a property that did not exist. The property not existing was the "programmer" error -- there should be code to handle that situation. If there had been feedback at the point of lookup (an exception, "property does not exist"), it would be easier to debug -- a simple matter of going to the line that generated the error, vs. having to backtrack the entire lifetime of the callee variable / object to figure out what's undefined. While it might be easy for us to debug now, due to practice, it is evidently not as simple as it could be, and it drives newcomers to JS from other languages up the wall. Node treating input type errors as operational errors would be the same sort of problem -- it would obfuscate real operational problems that need to be handled.

---

That all said (phew!), if there's a case where randomBytes is being given correct input and still throwing errors, then that is definitely worth investigating. Sorry for the wall of text, hopefully this clarifies how Node determines when and when not to throw errors, and the reasoning behind that.

[1]: Note, I'm a little squeamish about the term "programmer" error, but I'm using it to relate back to the linked article.