Skip to content

Commit 5e58b31

Browse files
committed
Fix MultiTransformIterator not destroying transformers on close
This was discovered in comunica/comunica#950 This may also be the root-cause of comunica/comunica#989
1 parent a536bca commit 5e58b31

File tree

2 files changed

+114
-0
lines changed

2 files changed

+114
-0
lines changed

asynciterator.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1506,6 +1506,16 @@ export class MultiTransformIterator<S, D = S> extends TransformIterator<S, D> {
15061506
if (!this._transformerQueue.length)
15071507
this.close();
15081508
}
1509+
1510+
protected _end(destroy: boolean) {
1511+
super._end(destroy);
1512+
1513+
// Also destroy the open transformers left in the queue
1514+
if (this._destroySource) {
1515+
for (const item of this._transformerQueue)
1516+
item.transformer.destroy();
1517+
}
1518+
}
15091519
}
15101520

15111521
/**

test/MultiTransformIterator-test.js

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,4 +386,108 @@ describe('MultiTransformIterator', () => {
386386
});
387387
});
388388
});
389+
390+
describe('A MultiTransformIterator with transformers that never end', () => {
391+
let iterator, source, multiTransform;
392+
beforeEach(() => {
393+
source = new ArrayIterator(['a', 'b', 'c', 'd', 'e', 'f']);
394+
multiTransform = sinon.spy(item => {
395+
const transformer = new BufferedIterator();
396+
scheduleTask(() => {
397+
transformer._push(`${item}1`);
398+
transformer._push(`${item}2`);
399+
transformer._push(`${item}3`);
400+
});
401+
return transformer;
402+
});
403+
});
404+
405+
describe('with autoStart and with destroySource', () => {
406+
beforeEach(() => {
407+
iterator = new MultiTransformIterator(source, { multiTransform, autoStart: true, destroySource: true });
408+
});
409+
410+
it('should destroy the transformers in the queue when closing', async () => {
411+
// Wait until the iterator has produced one result
412+
await new Promise(resolve => iterator.on('data', resolve));
413+
414+
// Immediately close the iterator
415+
iterator.close();
416+
417+
// Wait until the iterator has ended
418+
await new Promise(resolve => iterator.on('end', resolve));
419+
420+
// All transformers in the queue must have been closed
421+
iterator._transformerQueue.length.should.equal(4);
422+
for (const item of iterator._transformerQueue)
423+
item.transformer.closed.should.be.true;
424+
});
425+
});
426+
427+
describe('without autoStart and with destroySource', () => {
428+
beforeEach(() => {
429+
iterator = new MultiTransformIterator(source, { multiTransform, autoStart: false, destroySource: true });
430+
});
431+
432+
it('should destroy the transformers in the queue when closing', async () => {
433+
// Wait until the iterator has produced one result
434+
await new Promise(resolve => iterator.on('data', resolve));
435+
436+
// Immediately close the iterator
437+
iterator.close();
438+
439+
// Wait until the iterator has ended
440+
await new Promise(resolve => iterator.on('end', resolve));
441+
442+
// All transformers in the queue must have been closed
443+
iterator._transformerQueue.length.should.equal(4);
444+
for (const item of iterator._transformerQueue)
445+
item.transformer.closed.should.be.true;
446+
});
447+
});
448+
449+
describe('with autoStart and without destroySource', () => {
450+
beforeEach(() => {
451+
iterator = new MultiTransformIterator(source, { multiTransform, autoStart: true, destroySource: false });
452+
});
453+
454+
it('should destroy the transformers in the queue when closing', async () => {
455+
// Wait until the iterator has produced one result
456+
await new Promise(resolve => iterator.on('data', resolve));
457+
458+
// Immediately close the iterator
459+
iterator.close();
460+
461+
// Wait until the iterator has ended
462+
await new Promise(resolve => iterator.on('end', resolve));
463+
464+
// All transformers in the queue must not have been closed
465+
iterator._transformerQueue.length.should.equal(4);
466+
for (const item of iterator._transformerQueue)
467+
item.transformer.closed.should.be.false;
468+
});
469+
});
470+
471+
describe('without autoStart and without destroySource', () => {
472+
beforeEach(() => {
473+
iterator = new MultiTransformIterator(source, { multiTransform, autoStart: false, destroySource: false });
474+
});
475+
476+
it('should destroy the transformers in the queue when closing', async () => {
477+
// Wait until the iterator has produced one result
478+
await new Promise(resolve => iterator.on('data', resolve));
479+
480+
// Immediately close the iterator
481+
iterator.close();
482+
483+
// Wait until the iterator has ended
484+
await new Promise(resolve => iterator.on('end', resolve));
485+
486+
// All transformers in the queue must not have been closed
487+
iterator._transformerQueue.length.should.equal(4);
488+
for (const item of iterator._transformerQueue)
489+
item.transformer.closed.should.be.false;
490+
});
491+
});
492+
});
389493
});

0 commit comments

Comments
 (0)