Should take() close the iterator when remaining is 0?

[senocular]

take() returns undefined when remaining is 0 without calling back up to close the iterator. I'd expect iterator to get closed when this happens. For example

function* readLines() {
  try {
    open()
    while(1) yield read()
  } finally {
    close()
  }
}

// manual 
let count = 3
for (let line of readLines()) {
  // use line
  if (--count === 0) break; // closes
}
  
// using take()
for (let line of readLines().take(3)) {
  // use line
}
// not closed
  

[devsnek]

you can still use the iterator afterwards though.

[bergus]

I agree it should be closed. @senocular gave the most common use case for take(), and the iterator is normally not used afterwards. Closing resources is important here.

If people do want to keep using the iterator, they should need to specify explicitly that they don't want to close it already:

const iterator = readLines();
for (let i=0; i<3; i++)
  const line = iterator.next().value;
  // use line
}
const fourthLine = iterator.next().value;
iterator.return(); // close explcitly

const iterator = readLines();
let count = 3;
for (const line of notClosing(iterator)) {
  // use line
  if (--count === 0) break;
}
const fourthLine = iterator.next().value;
iterator.return(); // close explcitly

const iterator = readLines();
for (const line of notClosing(iterator.take(3))) {
  // use line
}
const fourthLine = iterator.next().value;
iterator.return(); // close explcitly

So this notClosing helper is necessary anyway for using an iterator multiple times. We could integrate it with take though, as an optional second parameter closes = true that can be used in for (const of iterator.take(3, false)).

[benjamingr]

We are actually running into the reverse of this in nodejs/node#46980 and it seems like in practice the expectation is for the iterator not to be closed to enable cases like:

const iter = getIterator();
doWorkOn(iter.take(4));
doWorkOn(iter.take(4)); // next 4, etc

[bakkot]

Hmm. I think a "get the next N items without closing the iterator" operation might be useful, but I'm not sure "take" is the appropriate name for it. Note that if the "doWorkOn" function in that example doesn't run to completion immediately you're going to get completely the wrong behavior - take isn't eager, so it only pulls from the underlying iterator when it itself is pulled from.

I see where the confusion there is coming from, though. Honestly limit might have been a better name than take...

[bakkot]

A second parameter as discussed above does seem reasonable though.

[benjamingr]

.take for taking and leaving the iterator open and .limit to only take n items and then close the iterator sounds like a good idea, good dsuggestion.

[zloirock]

const iter = (function * (i) { yield ++i })(0);
const iter1 = iter.take(4);
const iter2 = iter.take(4);
iter2.next().value; // do you expect 1 or 5?

[rluvaton]

const iter = (function * (i) { yield ++i })(0);
const iter1 = iter.take(4);
const iter2 = iter.take(4);
iter2.next().value; // do you expect 1 or 5?

I think 1 is the more correct behavior

[Mr0grog]

@senocular gave the most common use case for take(), and the iterator is normally not used afterwards. Closing resources is important here.

Is this is the most common use case? It is definitely very common, but wanting to do something with the first N items and something else with the rest like @benjamingr is suggesting is also something people do a lot. It’s certainly a pattern I have needed many times.

I think a "get the next N items without closing the iterator" operation might be useful, but I'm not sure "take" is the appropriate name for it.

Yeah I think I agree. @zloirock’s example highlights how you can easily get into problems using take() to solve this use case. If doWorkOn() in @benjamingr’s example doesn’t consume the iterator passed to it immediately, The second take(4) won’t produce what you expect (which is intuitively 5, 6, 7, ...). And when the actual consumption happens in a different function like @benjamingr’s example, the issue will be much harder to diagnose.

(That said, having take(n) close the iterator when it’s done won’t actually solve the problem in @zloirock’s example, right? It still won’t close the iterator until it’s been read from N times, which doesn’t happen in the example. You’d still have the problem, just in a slightly different way — iter1 might discover iter is already closed by the time it is read from.)

Anyway! A function that returns two iterators (one for the first N items and another for the all the items after) might be more appropriate for that use case. For example:

const [first4, tail] = iter.splitAt(4);
first4.next().value === 1
tail.next().value === 5

(Other names for similar operations in other languages/tools include partition and divide.)

Or maybe a second argument to .drop(n) that is a function to receive an iterator of the dropped items. That would let you chain more nicely, but might be a little weird:

iter
  .drop(4, (first4) => {
    first4.next().value === 1
  })
  .map((item) => {
    // item = 5, then 6, then 7, ...
  });

Either of those gives people a suggested solution for needing to do one thing with the first N and something else with the rest, leaving take(n) free to close the underlying iterator when it’s done (which seems like a good thing since people will often naively call iter.take(4) and forget to close the iterator).

[rluvaton]

What do you guys think of having an option to take?

[benjamingr]

I think changing take -> limit like @bakkot suggested to make the behavior explicit since take does other things on other platforms and libraries is a good idea. I also think and adding a splitAt like @Mr0grog suggested would be a good resolution. What do you think @devsnek ?

[bakkot]

splitAt

splitAt fundamentally can't work because you can't advance the iterator for the second part until the iterator for the first part has finished. Iterators which support splitting are a different thing and require a different interface. (Java calls it Spliterator.)

You can have a "eagerly get me the first N items" method (i.e. just call .next up to N times and wrap the results in an array), which leaves the underlying iterator in the state that it now gives you the items following that. But you can't split into two iterators, at least not without more support from the underlying iterator.

take does other things on other platforms and libraries

It doesn't, really - take on an iterator always means "give me an iterator over the first N" items. See rust, scala, etc.

It's just that only JS really has a notion of automatically closing an iterator when you stop iterating it before it ends, so only JS has to answer the question of whether the underlying thing gets closed when you exhaust the result of take.

[rluvaton]

closing after take is important to avoid memory leaks

Maybe @ronag and @mcollina have some input?

What do you guys think?