Support promises in the fs module

[bajtos]

This the first experimental step towards fully promise-enabled core API, see #11. The solution presented in this pull request has the following goals:

• A promise API that works with ES6 generators (e.g. via co) and that will work with ES7 async/await.
• The implementation is fully backwards compatible (with a small edge case - see below).
• The performance of existing callback-based code is not affected.
• The performance of promise-based code is reasonably good considering the the current state of Promise implementation in V8.

The promise API is disabled by default, one has to run node --promisify-core to enable it.

Note: I am intentionally focusing on the API where the conversion to promises is straightforward. Areas like streams and event-emitters, where the desired API is not clear yet, are completely out of scope of this work.

Tasks

• Initial spike to discuss the overall implementation (fs.stat, fs.readFile)
• Modify remaining functions: access, exists, close, open, read, write, rename, truncate, ftruncate, rmdir, fdatasync, fsync, mkdir, readdir, fstat, lstat, readLink, symlink, link, unlink, fchmod, chmod, fchown, chown, utimes, futimes, writeFile, appendFile, realPath
• Final code cleanup, remove possibly temporal entities like makeCallback vs. makeCallbackOrPromise.

Out of scope: fs.watch, fs.unwatchFile.

Breaking change

At the moment, when an async function is invoked without a callback, it will:

• silently discard the result on success
• throw an error on failure, this error goes to "unhandled error"
  handler (domain or process)

The PR changes the error behaviour: the rejected promise will be silently discarded, because the current version of V8 does not provide any way for detecting rejected promises that don't have any catch handler.

In the future, if V8 and/or the Promise standard provide a way for detecting unhandled rejections and Node integrates that mechanism, then this behaviour will be automatically reverted back.

Performance

Callback-based code is not affected by the change (even when --promisify-core is turned on), promise-based code is approx. 2x slower.

Benchmark results from my machine (rMBP; 2.7 GHz Intel Core i7; 16 GB 1600 MHz DDR3) are below, you are very welcome to run the benchmark yourself too. The rate "0.0000" means that a configuration was skipped as not available.

$ out/Release/node benchmark/fs/stat.js
fs/stat.js dur=5 concurrent=1 variant=promise: 0.0000
fs/stat.js dur=5 concurrent=1 variant=callback: 14944
fs/stat.js dur=5 concurrent=10 variant=promise: 0.0000
fs/stat.js dur=5 concurrent=10 variant=callback: 38148
fs/stat.js dur=5 concurrent=100 variant=promise: 0.0000
fs/stat.js dur=5 concurrent=100 variant=callback: 38725

$ out/Release/node --promisify-core benchmark/fs/stat.js
fs/stat.js dur=5 concurrent=1 variant=promise: 14651
fs/stat.js dur=5 concurrent=1 variant=callback: 16885
fs/stat.js dur=5 concurrent=10 variant=promise: 19011
fs/stat.js dur=5 concurrent=10 variant=callback: 38020
fs/stat.js dur=5 concurrent=100 variant=promise: 19353
fs/stat.js dur=5 concurrent=100 variant=callback: 41552

Additional information

Before I started the implementation, I run a benchmark to compare different ways of adding promises to fs.stat. See the following gist for more details: https://gist.github.com/bajtos/f8fca4fad36ada8e21dc

An example of using the new API with co and yield:

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

co(sourceAndStat);

function* sourceAndStat() {
  var stat = yield fs.stat(__filename);
  console.log('FILE SIZE: %s bytes\n', stat.size);
  var source = yield fs.readFile(__filename, 'utf-8');
  console.log(source);
}

---

@piscisaureus @bnoordhuis @trevnorris PTAL and let me know your opinion. I'd like to iron out the overall design before I start modifying the remaining 30 functions and writing unit-tests for them.

Things to consider:

• Is the option name --promisify-core revealing the intention? Should it be documented via node --help as proposed here, or should it be an undocumented flag?
• Testing strategy: How much tests do we need for the promise version of the API? Do we need to cover all edge cases, or is it enough to cover the most important paths (success, failure, optional arguments)? Is it better to mix promise tests with regular tests (as in test-fs-stat.js) or move them to a standalone file (test-fs-readfile-promise.js)?

cc: @sam-github

[rvagg]

I wouldn't get too excited here @bajtos, in case you missed the tone of the TC statement, this is not going to make it in to core any time soon. Your best bet might be to release your own promisified version or at least an npm package that does the wrapping so that you can demonstrate what it might look like. Otherwise you might be dealing with a very long running pull request.

[zensh]

@rvagg 👍

[bajtos]

I wouldn't get too excited here @bajtos, in case you missed the tone of the TC statement, this is not going to make it in to core any time soon.

I am aware of the TC statement, I double-checked my understanding with @piscisaureus.

I understand that promise-based API is not going to be a first-class citizen of node core anytime soon, and that's why I am proposing to put the promise-based API behind a flag. This is the same principle that was used for ES6 generators in node v0.11 and that is still used for most of other ES6 features in io.js. Because they are behind a flag, people understand they are not ready for general use. However, by having them shipped in node core, we are sending a strong signal that promises will be part of the core API in the future and providing a single API that people can start building on. By putting the code in core, we create a single place where people interested in promise-based API can collaborate.

[rvagg]

Generators are/were behind a flag because that's the default for V8, it wasn't a node-core decision.

IMO this is very premature, we're yet to see how Promises can be of material benefit when coupled with JS features because those features don't even exist yet. Right now they are simply sugar, an abstraction that many people prefer but still an abstraction and nothing more.

As we're still talking about an abstraction, this doesn't belong in core and can just as easily be provided as a package delivered via npm where it can be experimented on and iterated on there, just like streams2 was with readable-stream. Whether or not it makes it in to core will depend on how ES7 shapes up and how Promises fits in to the picture and whether it simply remains sugar.

[phpnode]

hmm. promises are not sugar, they provide material benefits for control flow and error handling that are virtually impossible to achieve with plain callbacks. They are already part of the language and it's quite strange to see the TC's reluctance to accept that fact. When async and await are finalized they will be powered by promises, and that will be transformative for node so I think it makes sense to start biting this particular bullet now. I like @bajtos's idea of including this behind a flag,

[rvagg]

When async and await are finalized

And then when V8 gets these features ... we haven't even got the full set of ES6 features yet, hence my caution about getting too excited.

I'll resist the additional trollbait in there.

[piscisaureus]

I understand the sentiment to tell people "go do this in user-land" when there's no strict need for something to be in core. In this particular case that adds nothing because it has been done over and over again.
Just from the top of my head: co, Q, streamline, AwaitScript, pn, tameJs, task.js, bluebird, node-fibers, gen-run. Those are all user-land efforts for making working with callbacks easier with either promises, or promises plus some form of await.
Some observations:

• Q and Bluebird pretty much offer the same functionality, and both implement superset of ES6 promises.
• Recent versions of co are pretty much indistinguishable from task.js; both emulate async-await using generators, yield and promises, with similar semantics.
• pn supports ES7 async-await by doing a source code transform; it's like a modern version of streamline and awaitscript, but using ES7 await semantics.
• All these libraries need to promise-ify node APIs, which is why there is a significant ecosystem of co-something and Q-something packages. pn doesn't have this ecosystem because they just built promise wrappers for node APIs into the library itself.

My interpretation is that the limit of userland experimentation has been reached, and node/iojs now just needs to show what the way forward is.

Off-topic maybe, but imagine that module support wasn't added in the very early days of node. Would we be ready to add a module system now, or would we tell people to experiment with user-land module loaders in eternity?

It's not a secret I dream of async-await wunderland, while there are others who get nauseous from just hearing the word Promises.
But it's important that we make decisions based on facts and not emotions. Will async-await really make people's lives easier? Will it make io.js unmaintainable? Are promises slow? Will they break backwards compatibility?
That's stuff I want to figure out, which is why I asked @bajtos to work on this PR.

And it helped - we know a little bit more now:

• With some generic glue, promise-ifying an individual async method can be done by adding three lines of code: https://github.com/iojs/io.js/pull/173/files#diff-9a205ef7ee967ee32efee02e58b3482dR819
• Promise-based fs.stat() is about twice as slow as a callback-based implementation.
• Promises can be supported without breaking backwards compatibility and without adversely affecting performance when callbacks are used.

That's progress.

Next steps, IMO, should be:

• As the TC, decide whether this first experiment has revealed any dealbreakers, or things to wait for.
• Ask the v8 team whether there are any plans to support async/await.
• As maintainers, look at @bajtos code and ask ourselves if this is an acceptable way of adding promise support. Maybe we can come up with a nicer way.
• As the TC consider whether adding an unstable feature behind a flag is an acceptable way of doing experiments.
• As a TC think of a reliable way to gauge community support for a particular feature.

I also agree that this PR will be open for a long time. This is out of scope for iojs 1.0.
I'd say that's a good reason to start early.

[hax]

Totally agree with @piscisaureus !

[bjouhier]

+1 @piscisaureus

Today we have a proliferation of tools / libraries to handle async. If async/await comes to life in ES7, the async/await + promises combination will supersede the solutions that we have today so there is a real opportunity for alignment. It would be a shame that we miss this opportunity.

If we align behind this proposal, we will have a very simple API principle:

• if you pass a callback, the API will use it to signal completion or error, as it has always been doing.
• if you omit the callback a promise will be returned.

This does not break compatibility (*) and it does not impact performance for callback-based code because the promise is not allocated if the function is called with a callback.

(*) except with some crypto calls that execute sync if the callback is missing but we don't need to break these calls, we can handle these special cases differently (with a wrapper module or alternate function names).

Some people have a strong case against promises or other forms of async tools. This is understandable as callbacks are probably the best fit for what they do and because performance is critical for them. BUT there are also people, especially on the applicative side, who are working in areas where callbacks are problematic and who are ready to trade a bit of performance for usability. If this was not the case, why would there be so many tools and so much debate? Node should cater for both and this is an opportunity to do so.

Side note: streamline works directly with existing node.js APIs so it does not need any of this. I'm not pushing my own tool here. Streamline is a stopgap solution until async/await gets baked into the language and available everywhere (node but also browsers) and the opportunity is to align on async/await + promises, not streamline. The irony is that modules developed with streamline today already follow the dual callback/promise API principle which is being proposed here. See https://github.com/Sage/streamlinejs#interoperability-with-promises.

[othiym23]

I am agnostic when it comes to promises, but I think that the dividing line of "put behind a flag because that's how V8 upstream shipped it" and "put behind a flag because we want to encourage experimentation using code that we wrote but aren't ready to support" is clear to Node maintainers, but seems kind of arbitrary when looked at from just outside. I think @piscisaureus has the right idea, and I think @bajtos's results thus far are very useful for framing the tradeoffs of doing this for real.

I also think that actual proactive efforts by Node's maintainers to align the platform with new ES features as they approach standardization would be a huge win to the entire JavaScript community. People already use Node + transpilers and shims as a testbed for new ES features, and if Node were to become more like Firefox, where the standards committee can look to it for real practitioner experience to guide the design of new features, we'd all benefit.

I'm sounding disturbingly like @domenic now. :/

[0b10011]

Should be second(data) instead, since you're returning data in the promise. Discarding the result immediately with callbacks skews the results slightly. (Same for third() and cb() calls below.)

See http://jsperf.com/callback-return-vs-no-return

[bajtos]

I have fixed the problem in f48e8fa. Can you please confirm that the current version is what you have meant?

[0b10011]

Perfecto!

[benjamingr]

It's worth mentioning this is way slower than Bluebird promisification because of the closure allocation.

[benjamingr]

@piscisaureus

Q and Bluebird pretty much offer the same functionality, and both implement superset of ES6 promises.

Bluebird is a much much more sophisticated library - it has major gains in performance, debuggability, maintainability of code and so on. I prey to God Petka doesn't see your message and take offence :)

With some generic glue, promise-ifying an individual async method can be done by adding three lines of code: https://github.com/iojs/io.js/pull/173/files#diff-9a205ef7ee967ee32efee02e58b3482dR819

The approach taken here is extremely naive. In order to promisify fast an approach can be taken that is much faster with almost zero overhead. However the faster approach - if we want to use native promises - would require help from v8. Help which Google people (@paulirish, please confirm :) ) said they're interested in offering if io.js makes concrete and reasonable requests.

Promise-based fs.stat() is about twice as slow as a callback-based implementation.

With a less naive approach it could be much faster. In some promise libraries allocating a promise is cheaper than allocating an array, and much cheaper than allocating a closure.

---

What I'm trying to say here is that the theoretical performance of promises should not be a concern here.

[YurySolovyov]

+1 with @benjamingr, I think we should call @petkaantonov to take a look, as he done great job working on bleubirds's promisification feature.

[petkaantonov]

I don't think it's possible without compromising the security that es6 promises are specced to have. Also io.js only uses V8's API, not internals.

Anyway, with an internal promisifier it's not only possible to skip allocating 3 closures (and since fs supports context passing there is no need to allocate any closure at all) but it also enables skipping the microtask trampoline for the promise and its children + follower promises when it's known that the callback is guaranteed to be async. The latter optimization has been done in bluebird 3.0 and it gave substantial improvement even after I thought there was nothing more to optimize.

Given non-secure promises, context-passing support in the callback API (such as fs) and an internal promisifier, the overhead over callbacks is virtually non-existent.