Repository navigation
Support Symbol.asyncIterator #254
Description
Async iterators are a stage 3 proposal for asynchronous pull streams (link) with a v8 implementation arriving very soon.
A new Symbol.asyncIterator is introduced. I believe stream.Readable should support Symbol.asyncIteator which would allow it to be iterated with for...await.
Supporting Symbol.asyncIterator means that readable streams are iterable through the new for await loops. ref.
Meaning we'll be able to do
let body = '';
for await(const data of req) body += data;
const parsed = JSON.parse(body); // note that we can let errors propagate, since we're in an async fn
console.log("got", parsed);Semantics
The async iterator protocol looks like:
interface AsyncIterator {
next(value) : Promise<IteratorResult>
[optional] throw(value) : Promise<IteratorResult>
[optional] return(value) : Promise<IteratorResult>
}
interface IteratorResult {
value : any
done : bool
}Which is similar to the regular JavaScript iteration protocol except that it returns promises. Since it is a pull stream supporting back pressure is pretty simple (since the consumer asks for values rather than being pushed values).
You .next the async iterator to get a promise for the next value in the iteration. You also can build async iterators with async generators. Here are some examples for both from a talk I gave: https://docs.google.com/presentation/d/1r2V1sLG8JSSk8txiLh4wfTkom-BoOsk52FgPBy8o3RM
Implementation
We'll add a Symbol.asyncIterable to readable streams that returns an object with next throw and return methods.
nextis called by thefor awaitloop to get the next value, it expects a promise to be returned that resolves with adone/valuepair. This should likelyreturnis called when a user abruptly completes the loop by callingbreak. This is our chance to do cleanup - like what you'd usefinallyfor and it marks completion (likedone: true).throwis on the consumer's side to propagate errors to the async iterator. I'm not sure what we should do here.
With regards to backpressure - the "easiest" thing would be to pause the stream whenever we just gave a value and unpause it on next - I'm concerend if that might affect performance even further.
Note that while there are interesting problems to solve with async iterators in general - I think our initial focus should be users of the new for await loop.
Performance
Allocating a promise for each value in next sounds prohibitively expensive. However, I am optimistic this will be solved in V8 land and the promise resolution will be inlined.
I recommend we start with a simple implementation and work our way towards a more optimized one.
Activity
benjamingr commentedon Dec 31, 2016
I'd also like to point out I'm far from being a NodeJS streams expert :)
caitp commentedon Jan 1, 2017
Note that this idea isn't guaranteed to work :) it could generate fatter code that simply isn't worthwhile.
Best case, you'd still have a property load for
then, and allocation of two different microtask entries, depending on which path is taken, which might not be worth it to elide the Promise allocation.It's something to try in 2017 though.
mcollina commentedon Jan 1, 2017
Before adding this to core, I think we should experiment with this in userland, even through the nodejs org. It's simple to polyfill, and I can think 2-3 different ways to implement it.
I recommend we experiment with the idea, and come up with a benchmark. This will help everyone assess the situation, before adding something to core that we will have to support forever.
I'm happy to produce that impl, if someone can help set it up the whole transpiling chain to try it.
calvinmetcalf commentedon Jan 6, 2017
I think I may have written an implementation of this lemme check
calvinmetcalf commentedon Jan 6, 2017
false alarm, I have not, yet
calvinmetcalf commentedon Jan 6, 2017
though more generally I'd vote for waiting and seeing what whatwg streams do re async iterators before we do anything
mcollina commentedon Jan 6, 2017
I generally agree. However, we should check if that API works for us before its finalized. After which, we do not have much chance to change things.
calvinmetcalf commentedon Jan 6, 2017
benjamingr commentedon Jan 6, 2017
@calvinmetcalf sounds good to me!
I think we should implement now (maybe behind a flag) and see how it works for us.
calvinmetcalf commentedon Jan 6, 2017
mcollina commentedon Jan 6, 2017
@calvinmetcalf yes. We can prototype it there with transpilers, and see how things are going. Then, import it here or within core.
calvinmetcalf commentedon Jan 9, 2017
prototype version, also this issue was originally nodejs/CTC#53
benjamingr commentedon Jan 9, 2017
@calvinmetcalf can you upload to NPM? I want to make sure I play with it and have feedback before the meeting.
mcollina commentedon Jan 9, 2017
I've opened up a couple of issues we have to discuss.
calvinmetcalf commentedon Jan 9, 2017
@benjamingr https://www.npmjs.com/package/async-iter-stream
2 remaining items
benjamingr commentedon Feb 25, 2017
I'm pretty content with the resolution and
async-iter-streamis satisfactory in the meantime and I'm enjoying it.I think we can close this until async iterators land in Node at which point we'll probably want to revisit this and add support to core :)
Thanks!
vsemozhetbyt commentedon Aug 20, 2017
A use case that will gain from iterable streams:
puppeteer/puppeteer#412
addaleax commentedon Aug 21, 2017
Fwiw this is available behind a harmony flag in Node 7+, so if we want to, we can start implementing things in core.
benjamingr commentedon Aug 21, 2017
See also discussion in nodejs/promises#31 regarding optimizing async iterators.
I think we should start implementing - but can't expect it to be fast any time soon.
mcollina commentedon Aug 21, 2017
When will async-iterators be outside of the harmony flag?
We have a working prototype https://github.com/calvinmetcalf/async-iter-stream, can you check if it works with
--harmony? I think it can help driving the discussion on how it can be made fast. However, I think it might be a bit too early for adding it into core IMHO.targos commentedon Aug 21, 2017
it was staged in V8 just a few days ago. That means the implementation is complete, which is great, but it can stay behind a flag for some time.
/cc @caitp Is there an intent to ship this soon?
targos commentedon Aug 21, 2017
@mcollina
async-iter-stream's test suite passes with both v8.4.0 and canary with--harmony-async-iteration(I executed test.js without babel-register)mcollina commentedon Aug 21, 2017
@targos this is amazing news.
caitp commentedon Aug 21, 2017
I've been asked to send one out recently.
This would give us some useful data for profiling, so the sooner the better :)
benjamingr commentedon Aug 21, 2017
When we ship, I don't mind writing a benchmark for readable streams as async iterators (vs callbacks with event emitters) in a real world app example (opening several streams and reading from there to fulfill a user request).
calvinmetcalf commentedon Aug 23, 2017
@benjamingr i'd be interested to see that, though they are going to have different perf characteristics because async-iterators support back pressure, though any help making async-iter-stream better would be very appreciated, I've got a few issues created in that repo
vsemozhetbyt commentedon Aug 25, 2017
Refs: https://medium.com/netscape/async-iterators-these-promises-are-killing-my-performance-4767df03d85b
mcollina commentedon Aug 25, 2017
@vsemozhetbyt I have just verified that https://github.com/mafintosh/csv-parser is as fast as the python alternative.
vsemozhetbyt commentedon Aug 25, 2017
Discussion in the proposal repository.