Skip to content

Commit

Permalink
Fix ClonedIterators not being readable
Browse files Browse the repository at this point in the history
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
  • Loading branch information
rubensworks authored and RubenVerborgh committed Jul 7, 2023
1 parent 9e80cc5 commit 3d9eecc
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 6 deletions.
3 changes: 3 additions & 0 deletions asynciterator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1796,6 +1796,9 @@ export class ClonedIterator<T> extends TransformIterator<T> {
constructor(source: AsyncIterator<T>) {
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() {
Expand Down
51 changes: 45 additions & 6 deletions test/ClonedIterator-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}

Expand All @@ -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', () => {
Expand All @@ -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', () => {
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 3d9eecc

Please sign in to comment.