Merge pull request #58 from nodelib/FG-282_fix_destroy_flow

FG-282: do not destroy reader when it is already destroyed

package.json

@@ -22,7 +22,7 @@
     "@nodelib-internal/tools.typedoc": "file:tools/typedoc",
     "@times-components/depend": "2.1.15",
     "@types/mocha": "^5.2.6",
-    "@types/node": "^11.13.0",
+    "@types/node": "^12.19.11",
     "@types/rimraf": "^2.0.2",
     "@types/run-parallel": "^1.1.0",
     "@types/sinon": "^7.0.11",

packages/fs/fs.stat/src/providers/sync.spec.ts

@@ -1,13 +1,15 @@
 import * as assert from 'assert';
 
+import * as sinon from 'sinon';
+
 import { Stats } from '../../../fs.macchiato';
 import Settings from '../settings';
 import * as provider from './sync';
 
 describe('Providers → Sync', () => {
 	describe('.read', () => {
 		it('should return lstat for non-symlink entry', () => {
-			const lstatSync = (): Stats => new Stats();
+			const lstatSync = sinon.stub().returns(new Stats());
 
 			const settings = new Settings({
 				fs: { lstatSync }
@@ -19,7 +21,7 @@ describe('Providers → Sync', () => {
 		});
 
 		it('should return lstat for symlink entry when the "followSymbolicLink" option is disabled', () => {
-			const lstatSync = (): Stats => new Stats({ isSymbolicLink: true });
+			const lstatSync = sinon.stub().returns(new Stats({ isSymbolicLink: true }));
 
 			const settings = new Settings({
 				followSymbolicLink: false,
@@ -32,8 +34,8 @@ describe('Providers → Sync', () => {
 		});
 
 		it('should return stat for symlink entry', () => {
-			const lstatSync = (): Stats => new Stats({ isSymbolicLink: true });
-			const statSync = (): Stats => new Stats({ ino: 1 });
+			const lstatSync = sinon.stub().returns(new Stats({ isSymbolicLink: true }));
+			const statSync = sinon.stub().returns(new Stats({ ino: 1 }));
 
 			const settings = new Settings({
 				fs: { lstatSync, statSync }
@@ -45,8 +47,8 @@ describe('Providers → Sync', () => {
 		});
 
 		it('should return marked stat for symlink entry when the "markSymbolicLink" option is enabled', () => {
-			const lstatSync = (): Stats => new Stats({ isSymbolicLink: true });
-			const statSync = (): Stats => new Stats({ ino: 1 });
+			const lstatSync = sinon.stub().returns(new Stats({ isSymbolicLink: true }));
+			const statSync = sinon.stub().returns(new Stats({ ino: 1 }));
 
 			const settings = new Settings({
 				markSymbolicLink: true,
@@ -59,10 +61,8 @@ describe('Providers → Sync', () => {
 		});
 
 		it('should return lstat for broken symlink entry when the "throwErrorOnBrokenSymbolicLink" option is disabled', () => {
-			const lstatSync = (): Stats => new Stats({ isSymbolicLink: true });
-			const statSync = (): never => {
-				throw new Error('error');
-			};
+			const lstatSync = sinon.stub().returns(new Stats({ isSymbolicLink: true }));
+			const statSync = sinon.stub().throws(new Error('error'));
 
 			const settings = new Settings({
 				fs: { lstatSync, statSync },
@@ -75,10 +75,8 @@ describe('Providers → Sync', () => {
 		});
 
 		it('should throw an error when symlink entry is broken', () => {
-			const lstatSync = (): Stats => new Stats({ isSymbolicLink: true });
-			const statSync = (): never => {
-				throw new Error('broken');
-			};
+			const lstatSync = sinon.stub().returns(new Stats({ isSymbolicLink: true }));
+			const statSync = sinon.stub().throws(new Error('broken'));
 
 			const settings = new Settings({
 				fs: { lstatSync, statSync }

packages/fs/fs.walk/src/providers/stream.spec.ts

@@ -10,10 +10,12 @@ import StreamProvider from './stream';
 
 class TestProvider extends StreamProvider {
 	protected readonly _reader: AsyncReader = new tests.TestAsyncReader() as unknown as AsyncReader;
-	protected readonly _stream: Readable = sinon.createStubInstance(Readable) as unknown as Readable;
 
 	constructor(_root: string, _settings: Settings = new Settings()) {
 		super(_root, _settings);
+
+		this._stream.emit = sinon.stub();
+		this._stream.push = sinon.stub();
 	}
 
 	public get reader(): tests.TestAsyncReader {
@@ -73,5 +75,16 @@ describe('Providers → Stream', () => {
 
 			assert.deepStrictEqual(provider.stream.push.args, [[null]]);
 		});
+
+		it('should do not destroy reader when it is already destroyed', () => {
+			const provider = new TestProvider('directory');
+
+			const stream = provider.read();
+
+			stream.destroy();
+
+			assert.ok(stream.destroyed);
+			assert.doesNotThrow(() => stream.destroy());
+		});
 	});
 });

packages/fs/fs.walk/src/providers/stream.ts

@@ -7,7 +7,11 @@ export default class StreamProvider {
 	protected readonly _stream: Readable = new Readable({
 		objectMode: true,
 		read: () => { /* noop */ },
-		destroy: this._reader.destroy.bind(this._reader)
+		destroy: () => {
+			if (!this._reader.isDestroyed) {
+				this._reader.destroy();
+			}
+		}
 	});
 
 	constructor(private readonly _root: string, private readonly _settings: Settings) { }

packages/fs/fs.walk/src/readers/async.spec.ts

@@ -211,6 +211,14 @@ describe('Readers → Async', () => {
 			reader.read();
 		});
 
+		it('should mark stream as "destroyed" after first destroy', () => {
+			const reader = new TestReader('directory');
+
+			reader.destroy();
+
+			assert.ok(reader.isDestroyed);
+		});
+
 		it('should throw an error when trying to destroy reader twice', () => {
 			const reader = new TestReader('directory');
 

packages/fs/fs.walk/src/readers/async.ts

@@ -41,6 +41,10 @@ export default class AsyncReader extends Reader {
 		return this._emitter;
 	}
 
+	public get isDestroyed(): boolean {
+		return this._isDestroyed;
+	}
+
 	public destroy(): void {
 		if (this._isDestroyed) {
 			throw new Error('The reader is already destroyed');