potential bug: `wrap` now autostarting iterators that were previously not autostarted prior to optimisation work
Closed this issue · 17 comments
Placeholder - will try create a minimal example or debug tommorow :)
On buggy case that I encountered today is the following which blocks on the second line
const iter = wrap(wrap([1, 2, 3], { autoStart: true }))
await iter.toArray()cc @jacoscaz
Note that I slightly changed toArray behavior in 174d87c: I found a bug where iterators that had already ended would cause a hang. Probably unrelated though.
Given the conversation around #45, should we address this before that merge or after?
I think before; this seems like a bug that we still want fixed in the current vminor.
@jeswr I'm not sure that is related to autoStart, actually. If autoStart is passed as an option to wrap(), regardless of its value, then the source is passed on to the TransformIterator constructor:
AsyncIterator/asynciterator.ts
Lines 2127 to 2128 in 915a857
However, with the current main the following doesn't work:
const iter = new TransformIterator([1, 2, 3])
const res = await iter.toArray();Therefore, I think what's wrong here is:
- the fact that we're passing an array as-is to
TransformIterator - the fact that we're not catching 1) as a mistake even though we're using TypeScript
Which is not to say that the premise of this issue is wrong. The default behavior if no autoStart option is provided might have changed, indeed.
- the fact that we're passing an array as-is to
TransformIterator
Indeed; that is not supported. I would have expected the type system to catch it, and TransformIterator to throw (because it cannot attach listeners).
I think the issue is at line 2128 (see above), which is in the body of wrap(). source (which might be an array) is being cast to MaybePromise<AsyncIterator<T>>, thus bypassing type checks in the TransformIterator constructor.
I think the intention of the original wrap typing was to not accept autoStart options for IterableSource, and thus only for AsyncIterator. So now the question is: do we adhere to the intended typings, or do we update the function such that options are possible also for IterableSource?
In any case, it does not seem like a regression because this would not have worked before the wrap rewrite.
IMHO and looking very long-term, the autoStart option should disappear (#25) . When working on new features I would try to limit support for it as much as possible while keeping backward compatibility when it comes to minor versions. I would adhere to the intended typings, disallowing autoStart for IterableSources.
for which kinds of arguments do we want to allow the autoStart option?
In #45 I renamed it to preBuffer (I think @RubenVerborgh s suggestion). It should only be available on iterators that extend the buffered iterator. When true the buffer will be pre-filled before the first .read() call; when false the buffer will remain empty until the first .read() call is made.
In the case of wrap I think we should remove the options parameter entirely to be completely honest. By setting maxBuffer or preBuffer users can suddenly introduce buffering even if they are just wrapping synchronous iterators.
I think it is better to force users to use the TransformIterator in the limited cases where they actually want to use it (I imagine there are less than a handful across all of Comunica where we would need to do this); and include a few docs on upgrading.
remove the options parameter entirely to be completely honest
But we can't for backward compatibility reasons.
It should only be available on iterators that extend the buffered iterator.
…and we probably don't want this kind of inheritance anymore, btw. I think we should do away with BufferedIterator as a base class, and only have it as a decorator where it is needed.
But we can't for backward compatibility reasons.
This suggestion was for the next vmajor
…and we probably don't want this kind of inheritance anymore, btw. I think we should do away with BufferedIterator as a base class, and only have it as a decorator where it is needed.
Yes, buffering and transforms are orthogonal concerns and addressing them independently of one another might be a good idea.