Consistent sync and async function names about node

[indutny]

Haha, let the discussion begin! :)

[calvinmetcalf]

I believe it's just crypto that still has problems right? Some places where async/non-blocking could help

• createDiffieHellman(length) this can take a very long time so a non blocking version of this would be helpful, as this is a constructor not a method it would require a bit more of an api tweak
• diffieHellman computeSecret, and generateKeys
• sign, verify, publicEncrypt, and privateDecrypt
• async, non-streaming encryption/decreption, authenticated encryption methods are fundamentally incompatible with streaming because you are supposed to authenticate before doing stuff with the data, a encrypt method which took an algorithm, data, password or key/iv and for gcm aad and a tag on decryption could have a sync an async versions (maybe slip in some stronger key derivation).
• non blocking hash stream? not sure if it would be much of a benefit.

[iefserge]

Would be nice to be able to call any async function as sync like this:

var sync = require('sync');
var fs = require('fs');

var stats = sync(fs.stat(file))

This could also work for user defined functions.

[vkurchatkin]

@iefserge related issue nodejs/node-v0.x-archive#7323.

Proof of concept 1: https://github.com/vkurchatkin/deasync
Proof of concept 2: vkurchatkin@5477d44

[nmn]

I think that while Node should be slowly cleaned up and made more consistent, it would be a mistake to change too much too soon in an overeagerness to improve things.

Instead, I think the best way forward would be to embrace the existing API and extend it with new features from ES6. (It's in everyone's best interest if Node.js stays consistent with the standards)

That said, renaming functions to make them more consistent with the general naming scheme is obviously a great idea. But let's not overhaul how sync and async functions work at the moment.

There are various ways to easily wrap packages as Promises or for generators. That is the best approach for now, as even though callbacks can be painful, they are the lowest level code possible, and can be wrapped to be compatible with Promises, Observables, Generators, Thunks and Channels. That flexibility is worth something.

[mikeal]

@iefserge that isn't actually possible. The best we could do would be

var sync = require('sync');
var fs = require('fs');

var stats = sync(fs.stat)(file)

[mikeal]

@Naman34 that is already happening above core, see: co and thunkify for instance.

In general, if something is being accomplished well in the ecosystem then core should stay out of it.

[yoshuawuyts]

@iefserge I'd argue that we probably need less synchronous functions, not more. Having a sync() function is extremely powerful, but ultimately I think it would tilt the platform in the wrong direction (e.g. away from being an async event machine).

Actually I'd be interested in what would happen if all sync functions would be removed entirely. I've had the impression many of the sync functions have been added just for the hell of it. Not sure if possible though, haha, mostly an interesting thought experiment.

[mikeal]

@yoshuawuyts the module system relies on a bunch of sync calls, so they can't be removed entirely.

[bnoordhuis]

createDiffieHellman(length) this can take a very long time so a non blocking version of this would be helpful, as this is a constructor not a method it would require a bit more of an api tweak
diffieHellman computeSecret, and generateKeys
sign, verify, publicEncrypt, and privateDecrypt

Agreed. This seems uncontroversial.

async, non-streaming encryption/decreption, authenticated encryption methods are fundamentally incompatible with streaming because you are supposed to authenticate before doing stuff with the data, a encrypt method which took an algorithm, data, password or key/iv and for gcm aad and a tag on decryption could have a sync an async versions (maybe slip in some stronger key derivation).

I'm not sure what you mean here. That you need to buffer before you can encrypt/decrypt/verify?

[iefserge]

@mikeal

that isn't actually possible

It's possible, but fs.stat needs to return some special async operation id value. And sync(id) blocks on that operation.
But var stats = sync(fs.stat)(file) would work as well.

[vkurchatkin]

fs.stat needs to return some special async operation id value

or simply a promise.

[darrenderidder]

Promises, like the many other async abstractions that exist, belong in user modules.

[nmn]

In general, if something is being accomplished well in the ecosystem then core should stay out of it.
@mikeal You basically said what I was trying to say in a concise way.

@darrenderidder: +1 to what you said.

I think sync function were added mostly for building command-line utilities. Let's not forget that node isn't ONLY for web servers.

[vkurchatkin]

Promises, like the many other async abstractions that exist, belong in user modules.

Yes, and that is why they are a part of the language

[iefserge]

@vkurchatkin Current promises implementation in v8 is very slow, too much overhead to create a promise for every async operation. Simple int id would work.

[nmn]

@vkurchatkin Promise is going to be part of the language soon. But it's not the right approach for all things yet. Promises suck for event handlers, like the callback for http.createServer.

Yes, there are other places where Promises are awesome to work with. But It's nice to have consistency in the API. And wrapping core features in Promises is SO TRIVIAL, it's not even worth messing with the core to save you a few lines of code per project.

Also, I really like what Pete Hunt said,
“Just because something is a standard, doesn't make it ‘the future’”

[vkurchatkin]

@iefserge I honestly don't think that promises can be a bottleneck. Eventually they could be fast and well optimized, so this is a sort of investment.

Your idea requires weak maps (I think), otherwise memory leaks are possible:

var registry = new WeakMap;

function asyncFn () {
   var id = Symbol();
   var operation = { done : false, result : null, error : null };
   registry.set(id, operation);

  someCallbackBasedFn(function (err, res) {
     operation.done = true;
     if (err)
      operation.error = err;
     else
       operation.result = res;
  });

  return id;
}

If we store registry as a Map or simple object we have to keep operation record indefinitely.

[vkurchatkin]

@Naman34 event listeners and cps callbacks are not the same. Listeners should be simple functions, no doubt. http.createServer is really a bad example: what's the point of making something an event if it is supposed to have a single listener? Better API (in my opinion) would be:

  var server = createServer(function (request) {
    return new Promise(function (resolve, reject) {
      resolve(new http.Response({/* some options */}))
    })
  });

[calvinmetcalf]

@bnoordhuis when you decrypt something with GCM, you shouldn't do anything with the decrypted data until you authenticate it, which means in practice you should buffer it on decryption.

[mikeal]

I honestly don't think that promises can be a bottleneck. Eventually they could be fast and well optimized, so this is a sort of investment.

Node doesn't intentionally make things slower. Ever. @trevnorris would literally burn this whole mother down, and with good reason.

There aren't actually any applications in Node that don't use third party modules outside of userland. The fact that promises can be accomplished easily in userland with no performance hit to the rest of core means they'll likely never be added to core itself. That doesn't mean people can't use them and build an ecosystem with them, you're more than welcome to, but it's silly to continue to ask for core to adopt them when there is a measurable penalty for doing so.

[jonathanong]

whoa whoa this issue for me was just about deprecating primarily crypto functions like crypto.randomBytes(length) in favor of crypto.randomBytesSync(length). nothing about different control flow mechanisms or new features.

[mikeal]

@jonathanong maybe you can update the title and description to be more specific and limit the scope :)

[jonathanong]

can we create a iojs/discussions repo for this type of control flow discussions?

[mikeal]

@jonathanong we're talking about core, it should probably be an issue in here.

[bnoordhuis]

this issue for me was just about deprecating primarily crypto functions like crypto.randomBytes(length) in favor of crypto.randomBytesSync(length)

That seems perfectly acceptable to me. I pulled the name out of thin air when I added crypto.randomBytes(), no thought was put into it. In retrospect, it's kind of an odd duck compared to other sync/async core APIs.

[trevnorris]

@mikeal Thanks. I need to write a post about core API and Promises/etc. that we can point to. This is a topic that's continuously brought up.

[trevnorris]

About adding API's like writeSync(), the problem is (as @piscisaureus pointed out to me) if there are asynchronous writes already in the pipeline to be written the sync call would need to pause execution until all async writes are complete. Doing that reliably is nothing trivial.

[jonathanong]

@trevnorris do you object to just making the crypto APIs consistent? if no one objects, i'll try to make a PR this weekend, but first i'll have to figure out how you guys do deprecation messages

[bnoordhuis]

but first i'll have to figure out how you guys do deprecation messages

exports.foo = util.deprecate(foo, 'use bar() instead');

git grep -w util.deprecate lib/ for more examples.

[sam-github]

e.g. crypto.randomBytes(24, callback) and crypto.randomBytes.sync(24)

If node was to start afresh, this could be a possibility, not particular opinion on its merits overall.

But with the way things work now ... the crypto API should converage on the naming conventions of other node core modules... not diverge even more wildly from them.

[seishun]

I don't like this direction. It would separate node functions into three categories:

1. I/O-bound functions: the normal version takes a callback, the *Sync version blocks and returns the value. Examples: fs.readFile/fs.readFileSync, fs.exists/fs.existsSync.
2. "Light" CPU-bound functions: only one version that just returns the value. Examples: url.parse, verifier.verify.
3. "Heavy" CPU-bound functions: the *Sync version returns the value, the non-Sync version takes a callback and does the task in a thread pool. Current examples: crypto.pbkdf2/crypto.pbkdf2Sync, zlib.deflate/zlib.deflateSync.

Discerning the first category is trivial if you know what the function does. The distinction between the last two, however, is quite arbitrary: for example, crypto.publicEncrypt is "light", while zlib.deflate is "heavy". There is no way to know whether the function takes a callback without looking it up.

Another problem is that *Sync has a stigma to it. Due to its meaning in fs, it looks like a red flag, signalling that it blocks and wastes time. On the contrary, deferring CPU-bound tasks to a thread pool is only beneficial if you're running a web server where handling requests involves I/O, since you want to start the I/O as soon as possible. Otherwise, it will only do harm because it's marginally slower and makes your code more convoluted, particularly when you want to handle things in a specific order.

TL;DR Let's not make select non-threadpool functions second-class citizens just because they have a threadpool counterpart.

[calvinmetcalf]

@caineio

1. yes
2. crypto
3. 1.0.0

[trevnorris]

@jonathanong I'm +1 on standardizing.

@seishun That is a valid concern, though I don't see much alternative to standardizing on an async/sync API and I don't see it as second-class. It's simply making a very clear delineation between a sync and async call.

[seishun]

@trevnorris personally, I don't see anything wrong with overloading based on an optional callback. It also has an added bonus that there will be no need to deprecate or rename anything if any existing sync-only CPU-bound functions ever get a threadpool version (e.g. crypto.publicEncrypt).

If you insist on separating them, then I would suggest appending "Async" to the threadpool functions instead, but I doubt many people would like this idea.

Is there a technical reason why exposing a generic wrapper for uv_queue_work is impossible/hard or a bad idea?

[trevnorris]

@seishun

Is there a technical reason why exposing a generic wrapper for uv_queue_work is impossible/hard or a bad idea?

If you mean to be able to execute JS in another thread, unfortunately it's much more complicated than that. I believe what you're asking is the same basic idea behind web workers API. Understand that to run JS in another thread you need to have an entirely different JS Isolate (which don't share heap or anything). There have been some ideas of how to implement this in Node, but no consensus has been reached about the best way to do this.

[Kagami]

For example, we should deprecate crypto functions like crypto.randomBytes(length) in favor of crypto.randomBytesSync(length).

Sorry for a bit offtopic question, but have you guys considered making crypto module more compatible with the WebCryptoAPI? E.g. make crypto.getRandomValues an alias for crypto.randomBytesSync? The current inconsistency (crypto.getRandomValues in browser and crypto.randomBytes in node) is a bit confusing.

[trevnorris]

@Kagami That would be a question for @indutny .

[seishun]

Here's another thought. If we go with the something/somethingSync convention, and later some existing sync-only function gets a threadpool version (say, publicEncrypt), then calling the non-Sync version of that function without a callback would be deprecated (but continue to work). Now what if io.js gets Promises out-of-box? Calling the non-Sync function without a callback would have to return a Promise, but that would break programs that expect a value.

[brendanashworth]

According to discussion here: #2170 (comment), it doesn't look like the crypto.randomBytes API is up for change like this.

#5 (comment) seems to be unrelated, so if this should be pursued, it should be pursued in a new issue / pull request.

I'm going to close this for now.