[oconnor663]

Fix "Barbara battles buffered streams" and iterator variants of "Futurelock" by adding poll_progress to AsyncIterator. Define a strict AsyncIterator contract for when poll_next and poll_progress should be called, mirroring the strict Future contract in RFC #4019.

To avoid burying the lede: Effectively deprecating StreamExt::next will probably be the most controversial part of this RFC. See "What about .next()?" in the Drawbacks section.

Important

Since RFCs involve many conversations at once that can be difficult to follow, please use review comment threads on the text changes instead of direct comments on the RFC.

If you don't have a particular section of the RFC to comment on, you can click on the "Comment on this file" button on the top-right corner of the diff, to the right of the "Viewed" checkbox. This will create a separate thread even if others have commented on the file too.

Rendered

[kpreid]

I think this is a noteworthy change to the big picture of async. Right now, when teaching how async works, it is possible to say (and I have said) that

async/await by itself does not add concurrency; the execution of your async code, and the things it awaits, remains fully sequential. Writing async code just makes it possible to suspend execution. All concurrency is introduced either by executors or by combinators that implement Future manually.

(This helps explain things to people who are trying to achieve concurrency and don’t yet understand why just putting async on everything doesn’t help — perhaps because they are familiar with JavaScript async, where every async function call effectively spawns itself.)

With this change, that will no longer be true; for await will, all by itself, introduce concurrency between the loop body and the iterator. This is probably worth the conceptual complexity, but I think it should be called out more explicitly in the RFC as a cost to understanding and teaching async.

---

It also might be interesting to design a more primitive concurrency construct that for await can be desugared to; if there can be such a thing, it might be useful to offer to users for situations that need this same kind of concurrency but don’t fit async for.

[oconnor663]

for await will, all by itself, introduce concurrency between the loop body and the iterator

If I understand you correctly, I think I disagree with this. If I write an async gen fn and then iterate over it with a for await loop ("just sprinkle some async on my code"), control will alternate between the two, and nothing will run concurrently. The observable behavior is the same as the non-async version. That's true of today's AsyncIterator/Stream, and it remains true under this RFC. I can't tell whether that was already clear.

What's different under this proposal is that, if I implement AsyncIterator manually -- similar to implementing Future manually like you mentioned -- I have the option of doing concurrent work in poll_progress. An async gen fn by itself won't take advantage of this option. If you want your for await loop or .for_each(...) closure to run concurrently with your async gen fn, you need to wrap it in an adapter like the Buffer1 example.¹ However, for await loops and other consumers will wire up poll_progress appropriately for every iterator they loop over, in case any of those happen to be concurrent iterators like Buffer1/Merge/FuturesUnordered. Those poll_progress calls mean we can't desugar for await into a loop without breaking short-circuiting operators, but in simple "sprinkle async on it" situations they're all no-ops,² and there isn't any real concurrent work.

The long discussion of the proposed "PollNext::Pending rule" (this section and the following section) is also relevant here, specifically:

there's a footgun: Map::poll_progress shouldn't always try to fill that buffer slot, or else the first loop above will start printing "NUMBER 1" while its body is sleeping.

Precisely because we don't expect ordinary async iterators to run concurrently with the loops that drive them, we need to think about how easy it is do that accidentally in manual implementations, which has implications for the details of the contract.

Footnotes

1. Writing an inherently concurrent async gen fn could be possible if we added something like the hypothetical await all syntax in the "Future possibilities" section. ↩

2. There's some discussion in the RFC about whether/when a generated poll_progress function should be literally a no-op that the optimizer can easily delete, vs something with asserts in it. I need advice here from folks with compiler experience. Maybe this is also something we can experiment with later without breaking backwards compatibility? Famous last words I know. ↩

[kpreid]

for await will, all by itself, introduce concurrency between the loop body and the iterator

If I understand you correctly, I think I disagree with this. …

What's different under this proposal is that, if I implement AsyncIterator manually -- similar to implementing Future manually like you mentioned -- I have the option of doing concurrent work in poll_progress. An async gen fn by itself won't take advantage of this option.

I see, and I agree with your logic for the case I wrote out — it will still be true that you cannot choose to add concurrency by just asyncifying things, and we will still need to teach people that.

However, I think it also matters for teaching and debugging purposes that in this new world, for await will be introducing a scope in which concurrency occurs, which is something that async/await syntax (and for syntax) has never done before. (Today, join(a, b).await is concurrent but the concurrency is between parts of the join, not the join and something else.) It means that, if the code containing the loop and the async iterator have some interaction the developer cares about (whether an explicit one via interior mutability, or an implicit one via timing or peak memory usage), then the developer needs to think about the code in the loop body differently from the code outside the loop body, in a way other than just “the loop body is executed more than once”.

I don’t think this is a harmful thing, but it’s something that should be acknowledged prominently in the design (which the guide-level explanation already does) and in documentation.

[tmccombs]

Ok, so the panic might, maybe catch some kinds of bugs. But:

1. panicking in this situation isn't guaranteed, it is also allowed to "misbehave" in other ways, which may be difficult to debug.
2. In more complicated combinators, the panic may not be consistently reached, say because poll_progress is only called while poll_next is pending in certain situations. Which leads to the panic not getting caught until the code is "in production".
3. I might argue that a panic is still a confusing control flow bug.
4. Not being allowed to call to poll_progress on an iterator that is already pending could potentially make some implementations more complicated. For example, the imlementation of poll_progress for a Merge iterator would need to keep track of which if any children are in the pending state to ensure it doesn't call poll_progress on those.

All in all, I think it would be better to define the semantics of calling poll_progress while poll_next is pending as one of the following:

1. (my preference) Return Poll::Pending, it may cause a spurious poll, but can also do nothing (a no-op).
2. Return Poll::Pending and do nothing else. calling it is harmless, but should not change the state or do work on the future.
3. Always panic. This is my least favorite, but at least the behavior is consistent.

For 1 or 2, the definition for Then::poll_progress would look like:

fn poll_progress(self: Pin<&mut Self>, cx: &mut Context) -> Poll<()> {
    if self.future.is_none() {
         self.project().stream.poll_progress(cx)
    } else {
        Poll::Pending  
    }
}

View changes since the review

[oconnor663]

panicking in this situation isn't guaranteed, it is also allowed to "misbehave" in other ways, which may be difficult to debug.

Note that this wording is modeled somewhat on our existing docs for Future::poll: "Once a future has completed (returned Ready from poll), calling its poll method again may panic, block forever, or cause other kinds of problems; the Future trait places no requirements on the effects of such a call." That doesn't make it necessarily the right thing to do, but I do think it makes sense for Future::poll to work that way.

In more complicated combinators, the panic may not be consistently reached

True. And continuing the analogy, there are also many futures that you can successfully poll even after they return Ready. Some document this as a feature (e.g. Fuse), and some don't (e.g. Sleep). However, the most common type of future is an async fn future, and the fact that those reliably panic means that most callers who make this mistake hit a panic quickly. I think if we do something similar with async gen fn, it will have a similar effect.

I might argue that a panic is still a confusing control flow bug.

For sure, I think panicking is just the lesser of a few evils here, and it's good and important that regular users writing regular async fn and async gen fn code won't be exposed to this stuff. Personally, the bug I'm most worried about is a combinator that violates the "PollNext::Pending rule" (like the "lazy" Merge implementation in the RFC) combined with an async iterator (like the "that's nice and simple, but is it correct?" Map implementation in the RFC) that's only correct if the rule is followed. That combination can cause hangs and deadlocks, and those will be murder to debug.

Not being allowed to call to poll_progress on an iterator that is already pending could potentially make some implementations more complicated.

Take a look at this section and the section after and tell me what you think about what I wrote there. In particular, I agree you that this rule makes Merge more complicated, but I'm worried that the alternative makes almost everything else more complicated.

[tmccombs]

I think it would be useful to see some example code of how AsyncIterator would be implemented on Merge (or FuturesUnordered) with this RFC.

View changes since the review

[oconnor663]

It's not in the text itself, but there's an example of implementing Merge in the fixed playground example in the Motivation section. Ctrl-f for "here's the sequence of events".