From 3d9eecca4b5fceb550f4e550f5c580ffbe8244f3 Mon Sep 17 00:00:00 2001 From: Ruben Taelman Date: Sun, 28 May 2023 16:22:34 +0200 Subject: [PATCH] Fix ClonedIterators not being readable Since ClonedIterators are not autoStarted, they should be marked as readable. Otherwise, dependents may never consider them as being readable. This lies at the root cause of the following Comunica bugs, where execution would halt due to an intermediary iterator never becoming readable: - comunica/comunica#1168 - comunica/comunica#817 --- asynciterator.ts | 3 +++ test/ClonedIterator-test.js | 51 ++++++++++++++++++++++++++++++++----- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/asynciterator.ts b/asynciterator.ts index 30e4358..8bea27d 100644 --- a/asynciterator.ts +++ b/asynciterator.ts @@ -1796,6 +1796,9 @@ export class ClonedIterator extends TransformIterator { constructor(source: AsyncIterator) { super(source, { autoStart: false }); this._reading = false; + // As cloned iterators are not auto-started, they must always be marked as readable. + if (source) + this.readable = true; } protected _init() { diff --git a/test/ClonedIterator-test.js b/test/ClonedIterator-test.js index d2fc372..9ab5c9a 100644 --- a/test/ClonedIterator-test.js +++ b/test/ClonedIterator-test.js @@ -173,8 +173,8 @@ describe('ClonedIterator', () => { }); if (index === 0) { - it('should not have emitted the `readable` event', () => { - getClone()._eventCounts.readable.should.equal(0); + it('should have emitted the `readable` event once', () => { + getClone()._eventCounts.readable.should.equal(1); }); } @@ -187,8 +187,8 @@ describe('ClonedIterator', () => { }); if (index === 0) { - it('should not be readable', () => { - getClone().readable.should.be.false; + it('should be readable', () => { + getClone().readable.should.be.true; }); it('should return null on read', () => { @@ -204,7 +204,7 @@ describe('ClonedIterator', () => { before(() => { getIterator()._push('a'); }); it('should have emitted the `readable` event', () => { - getClone()._eventCounts.readable.should.equal(1); + getClone()._eventCounts.readable.should.equal(index === 0 ? 2 : 1); }); it('should not have emitted the `end` event', () => { @@ -231,7 +231,7 @@ describe('ClonedIterator', () => { before(() => { getIterator().close(); }); it('should not have emitted anymore `readable` events', () => { - getClone()._eventCounts.readable.should.equal(1); + getClone()._eventCounts.readable.should.equal(index === 0 ? 2 : 1); }); it('should have emitted the `end` event', () => { @@ -756,6 +756,45 @@ describe('ClonedIterator', () => { }); }); + describe('Cloning an iterator without autoStart', () => { + const clones = createClones(() => new TransformIterator(() => new ArrayIterator([1], { autoStart: false }))); + + describe('before the first item is read', () => { + describeClones(clones, getClone => { + it('should be readable', () => { + getClone().readable.should.be.true; + }); + + it('should have emitted the `readable` event', () => { + getClone()._eventCounts.readable.should.equal(1); + }); + }); + }); + + describe('after the first item is read', () => { + describeClones(clones, getClone => { + let item; + before(() => { item = getClone().read(); }); + + it('should have read the item correctly', () => { + item.should.equal(1); + }); + + it('should not be readable', () => { + getClone().readable.should.be.false; + }); + + it('should be done', () => { + getClone().done.should.be.true; + }); + + it('should not have emitted another `readable` event', () => { + getClone()._eventCounts.readable.should.equal(1); + }); + }); + }); + }); + describe('Cloning an iterator that errors', () => { const clones = createClones(() => new AsyncIterator()); let iterator;