Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Establish convention about explicit/implicit synchronous function versions #7030

Closed
thomseddon opened this issue Feb 2, 2014 · 26 comments
Closed

Comments

@thomseddon
Copy link

I think there is a level of inconstancy in the current API with regards to the use of async/sync. In some places, async and sync version of functions are separate with async being default, in other places sync is default, in other async/sync can be implicitly implied depending on whether a callback is supplied.

Firstly, an overview of the current state:

  • crypto: All implicitly synchronous except pbkdf which is async by default but offers an explicit *Sync() version and randomBytes() where sync/async is implied depending on if a callback is supplied
  • fs: All implicitly async with explicit sync versions via corresponding *Sync() methods
  • zlib: Now, implicitly async/sync depending on if callback is supplied

Other modules, such as dns, http, net, os, path etc. are all implicitly sync or async and either do or do not accept a callback.

This was briefly brought up in #6987 and subsequently #7028 but I would like to nail this once and for all across all API's and have a definitive convention if possible?

Personally, I am in favour of depreciating all the explicit *Sync() functions and converging on a convention of async/sync being implicitly implied based on if a callback is supplied. Users would know if a function has async/sync versions based on if a callback is or isn't used respectively. Also, if a callback is not an option, it's always sync. Conversely, if it is required, it's always async with no sync option.
This would be relatively easily integrate into current fs module and would a solid convention moving forward?

@rlidwka
Copy link

rlidwka commented Feb 2, 2014

I would be in favor of deprecating all Sync functions and don't make any replacement for them.

a convention of async/sync being implicitly implied based on if a callback is supplied

If I write fs.writeFile("something", "file") without a callback, it doesn't mean I want it to be sync. It just means that I don't care about the result.

@jfhbrook
Copy link

jfhbrook commented Feb 2, 2014

I would be in favor of deprecating all Sync functions and don't make any replacement for them.

Insanity. There are good reasons for sync functions. Just look at require!

@jfhbrook
Copy link

jfhbrook commented Feb 2, 2014

Anyways, I'm preeeeeeetty sure this is a frozen API and it's never gonna change. So good luck with that.

@rlidwka
Copy link

rlidwka commented Feb 2, 2014

Insanity. There are good reasons for sync functions. Just look at require!

And after that look at how you can modify require to load files from a database or a remote server. Synchronous require is a bad idea.

Any other arguments?

@puzrin
Copy link

puzrin commented Feb 2, 2014

I agree, that multiple conventions across modules are confusing. Especially in crypto.

IMHO callback autodetection looks very attractive for convenient programming. And allows to keep API compatibility. If one care about v8 inline cache, he can always use implicit *Sync alternative to make sure that signatires in hot paths are monomorphic.

My suggestion is:

  • consider adding callbacks autodetection to modules
  • do *Sync implicit notation for compatibility & complex cases.

Are there some cases when user wish to make async call without waiting callback?

@thomseddon
Copy link
Author

To be clear, when I said depreciate *Sync() functions, I meant readFileSync() and friends, not all sync functions per say.

Good point by @rlidwka with regards to wanting to run a function async but not caring about the result, I think that is a very valid use case.

Perhaps if the signature was: fs.readFile(filename, [options], [callback]) one could still run it async like so: fs.readFile('myfile.txt', true)?

@vkurchatkin
Copy link

my vote if for explicit Sync everywhere, it's just easier to find it in code and is less error prone (that is, less chance to use sync without actually meaning to).

And after that look at how you can modify require to load files from a database or a remote server. Synchronous require is a bad idea.

@rlidwka that's not impossible, but everybody seems to be good with sync require

@puzrin
Copy link

puzrin commented Feb 2, 2014

IMHO having explicit Sync in crypto (md5Sync) is ugly and useless. That's why i suggested auto + Sync - to let everyone have his prefered style.

You rarely need to intensively mix different styles in one script. Usually you write something like service (with all calls async) or shell script (with all calls sync). Also, in most cases, such async calls are used with lambdas and it's next to impossible to miss it by accident.

@indutny
Copy link
Member

indutny commented Feb 2, 2014

+1 for Sync, but perhaps with leaving crypto as it is...

@jamescostian
Copy link

I think explicit Sync everywhere, except for crypto, would be the best because in the future when promises are enabled by default in node, running the async methods without a callback should return a promise. Some people disagree with this idea because they believe that it forces us to choose 1 of the many userland modules and incorporate it into core (an argument which I used to completely agree with), however, considering that the latest draft for es-harmony includes promises, I think that promises are just going to get more and more popular, and there's nothing wrong with using promises that will be native to JavaScript.

EDIT: Fixed link

@thomseddon
Copy link
Author

If it was *Sync() everywhere, what would the justification for it not being so in crypto?

I assume it is because there is currently no async alternative...I apologise for my ignorance but why don't async versions of the existing crypto methods exist?

@seishun
Copy link

seishun commented Feb 2, 2014

I'm not in favor of lumping together I/O-bound operations and heavy CPU-bound operations since they are very different. If you use, say, fs.readFileSync, your CPU will be basically doing nothing for some time, which is almost never a good idea. On the other hand, if you call zlib.deflate without a callback, the operation will complete faster (uninterrupted by other events), which in many cases is the desirable behavior.

I like the convention of using explicit Sync for I/O-bound operations and optional callback for CPU-bound operations. Since the former is dangerous, it should take more effort to type. The latter has its pros and cons, so the sync and async versions should be on equal footing.

@Fishrock123
Copy link

+1 for a move to more consistency.

@bjouhier
Copy link

bjouhier commented Feb 2, 2014

Completely with you @rlidwka

People who want sync-style should use the existing tools that solve this problem instead of asking for more Sync APIs.

And yes, require is the black sheep. Would be so cool if we could require modules from a database!

@mscdex
Copy link

mscdex commented Feb 2, 2014

+100 for appending 'Sync' to synchronous functions, no matter if it's I/O or CPU-bound, and no magical switching to sync on missing callback.

@jfhbrook
Copy link

jfhbrook commented Feb 2, 2014

Synchronous require is a bad idea.

There's an entire rich history of discussion and decisions you're ignoring here. There's a reason require is sync.

@rlidwka
Copy link

rlidwka commented Feb 2, 2014

There's a reason require is sync.

For every single decision there is a reason, even for wrong ones.

Take the recent commit for example, which directly contradicts node documentation. Is there a reason for it? Yes. Does it make it good idea? No.

@vkurchatkin
Copy link

@rlidwka do you mean this part?

It is very important for APIs to be either 100% synchronous or 100% asynchronous.

I think it only applies to asynchronous vs synchronous callback, not callback vs return.

@mks
Copy link

mks commented Feb 3, 2014

+1 for explicit > implicit. No magic in core.

@seishun
Copy link

seishun commented Feb 3, 2014

+100 for appending 'Sync' to synchronous functions, no matter if it's I/O or CPU-bound, and no magical switching to sync on missing callback.

Then we must append Sync to every single function and method in JavaScript, or be inconsistent. There's no fundamental difference between, say, JSON.stringify and synchronous zlib.deflate.

@vkurchatkin
Copy link

@seishun there is:

  • JSON.stringify is a part of the language and not a subject to change;
  • JSON.stringify doesn't have asynchronous counterpart.

But otherwise you are right, JSON.stringify can be pretty slow too.

@thomseddon
Copy link
Author

When #7035 get's merged the only inconsistency will be the crypto functions

@mscdex
Copy link

mscdex commented Feb 3, 2014

@thomseddon I partially agree. However currently most of the crypto functions do not have asynchronous counterparts. But methods like randomBytes() and pseudoRandomBytes() should have separate 'Sync' methods for the synchronous functionality.

@seishun
Copy link

seishun commented Feb 3, 2014

If you insist on separating sync and async versions of CPU-bound operations, then append Async to the latter instead. It doesn't make sense to apply a different standard to most of CPU-bound functions just because they don't have an asynchronous counterpart.

In Node, all I/O is by default asynchronous, so it makes sense to have special names for the only exceptions to this rule in fs. For CPU-bound operations it's the opposite – everything in JavaScript standard library, most of Node and anything you write yourself is synchronous, async randomBytes in crypto and zlib convenience methods are the exceptions here.

Also, treating CPU- and I/O-bound operations the same way sends a dangerous message to novice programmers. Since it's perfectly safe to have a hundred simultaneous HTTP requests, they might assume that it's also a good idea to compress a hundred files asynchronously and that it will magically be faster than doing it synchronously.

@jonathanong
Copy link

is splitting up crypto like @indutny did with zlib a contentious issue or a matter of making a PR?

@jasnell
Copy link
Member

jasnell commented Jun 22, 2015

Going to close this and defer any further conversation to the nodejs/api working group.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests