Should the Repeater automatically stop when its executor fulfills?

[brainkim]

Wrote some code in #48 that was like:

function map(iterable, fn) {
  return new Repeater(async (push, stop) => {
    const iter = iterable[Symbol.asyncIterator]();
    let finalIteration;
    stop.then(() => {
      finalIteration = typeof iter.return === "function" ? iter.return() : {done: true};
    });
    while (!finalIteration) {
      const iteration = await iter.next();
      if (iteration.done) {
        stop();
        return iteration.value;
      }
      await push(fn(iteration.value));
    }
    // there’s no need to return finalIteration’s value here because when repeaters are returned, the return value will just be whatever was passed into the `return` method.
    await finalIteration;
  });
}

Note that we have to call stop manually before performing an early return in the repeater executor (stop(); return;). I’m starting to think that the stop could just be implicit when we return from the executor.

We automatically stop the executor when the repeater execution rejects or throws, so why don’t we automatically stop the executor when it returns or fulfills? I think I initially did not implement repeaters in this way because of tests where I would push in a sync function, but I have never written any actual real-life repeaters which don’t await stop somehow.

This would be a breaking change. It would require people who returned early from repeaters to await stop, but I’m not sure anyone uses repeaters like that.

[n1ru4l]

As I am starting to use this library I was actually expecting this and it took me some time to realize that I have to add a stop call at the end of the executor function. If the AsyncIterable isnt disposed once the end of the executor is reached people can just add an await stop at the end of the executor function 🤔

[brainkim]

@n1ru4l This is one of those issues I’ve been thinking about on and off this late, wretched year. I do think there’s something aesthetically pleasing about keeping the repeater open only during the execution of the executor; nevertheless, my current thinking is that if we changed the behavior to auto-stop, it would become a common pitfall, as people might forget to leave the repeater open and wonder why the repeater had abruptly closed. I think auto-stopping would be tricky to debug if you were not familiar with the library. Therefore, I concluded that the current behavior is not that bad even if it is unexpected, and that if you want to end iteration, calling stop() isn’t a bad trade-off for the sake of explicitness and ease of use for new users. On top of that, not having to release another major version is always a plus.

I truly appreciate you sharing your experience, but I’m not sure if it’s reason enough to change the behavior of the library at this time. But I do like to keep an open mind. Maybe we can compile a pro-con list for auto-stopping the repeater and decide based on that.

[jedwards1211]

@brainkim this caught me by surprise too, I think it was causing my event loop to die, though I haven't gotten to the bottom of it. Event loop death is one of the most confusing things to debug in Node.js so I argue that keeping the repeater open should be opt-in behavior rather than opt-out. I think it will be easier for people who forgot to keep their repeater open will figure out their mistake than it is for people like me to figure out we explicitly have to call stop.

[jedwards1211]

@brainkim the docs are certainly unclear on this. This adds to the confusion:

If the executor returns normally, the return value of the executor is used as the final value of the repeater.

Also, nothing I see in the docs explicitly states that the repeater will stay open if stop is never called (potentially allowing the event loop to die).

[jedwards1211]

Another question: in practice are there many use cases where you would want to leave a repeater open as long as the consumer is iterating without needing to await stop and perform some kind of cleanup afterward?