Chain iterator adapter changed behavior in nightly

[arturoc]

Recently the Chain adapter has been changed to fuse the iterators it holds by destroying them, this introduces a subtle behavior change that is difficult to debug and as far as I see not documented.

For example the following:

use std::sync::mpsc;

fn main() {
    let (tx,rx) = mpsc::channel();
    let mut it = vec![1, 2, 3].into_iter().chain(rx.try_iter());
    
    tx.send(4).unwrap();
    tx.send(5).unwrap();
    assert_eq!(it.next(), Some(1));
    assert_eq!(it.next(), Some(2));
    assert_eq!(it.next(), Some(3));
    assert_eq!(it.next(), Some(4));
    assert_eq!(it.next(), Some(5));
    assert_eq!(it.next(), None);
    
    tx.send(6).unwrap();
    assert_eq!(it.next(), Some(6));
    
    tx.send(7).unwrap();
    assert_eq!(it.next(), Some(7));
}

runs correctly on current stable (1.42) but panics in nightly cause the iterator is now fused so it can't be called anymore after it returns None for the first time.

stable: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a36e52425fa425eac3b7c24341b73d49

nightly: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=4958f8dccb7594a0fa06a26ebc3d442b

Perhaps a possible solution would be to fuse the first iterator in the chain but not the second?

[Mark-Simulacrum]

cc @cuviper @scottmcm

[cuviper]

Recently the Chain adapter has been changed to fuse the iterators it holds by destroying them,

While the early drop is an observable change, I don't think that's directly your issue. You'd have the same problem if we just fused with a flag or a ChainState::Done. You're using try_iter() which only borrows the Receiver, and doesn't implement Drop. So in this case there's no functional difference between clearing our Some(iterator) to None, or just ignoring it from a flag.

Chain is further complicated by being double-ended, though that doesn't apply with Receiver. Even in the old code, the fusing behavior depended on which direction you moved from ChainState::Both. If you exhausted in the forward direction, then all further calls (forward or backward) would only try the second iterator (ChainState::Back); likewise if you exhausted backward, further calls only try the first iterator (ChainState::Front).

So I feel the current approach is more consistent, fusing both iterators in either direction, but I do see how that breaks your use case. ☹️

Perhaps a possible solution would be to fuse the first iterator in the chain but not the second?

We can do that, but I think double-ended chain will still be weird, and not quite the same weird as before. At first blush, I'd make forward methods only fuse the first iterator, and reverse methods only fuse the second iterator. If you have a mix of both directions, that would still make it possible to become totally fused, which is different than ChainState getting stuck to one side.

---

In defense of early dropping, I think not dropping can also cause hard-to-debug issues when one side is silently ignored. Consider the old ChainState, and suppose the first iterator was from Receiver::into_iter(). When we transitioned from Both to just Back, the first iterator would be ignored. However, the Sender could still feed items into that channel, and there was no way to receive them anymore. With early drop, the sender can at least see an error that the channel is closed.

[arturoc]

At first blush, I'd make forward methods only fuse the first iterator, and reverse methods only fuse the second iterator. If you have a mix of both directions, that would still make it possible to become totally fused, which is different than ChainState getting stuck to one side.

That sounds reasonable, the main thing I would take into account is apart from which implementation is the most correct, the fact that this change in behavior is probably going to break things in subtle ways that are hard to debug.

As you say I discovered this cause when the receiver dropped the sender got notified but i wasn't even using an mpsc channel but my own broadcast mpmc object so it just stopped receiving but the sender didn't notify me right away with an error cause other receivers where still alive which made it harder to figure out to the point where i had to modify the mpmc code to inspect the number of receivers still alive.