Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Dec 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -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",
Expand Down
26 changes: 12 additions & 14 deletions 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 }
Expand All @@ -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,
Expand All @@ -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 }
Expand All @@ -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,
Expand All @@ -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 },
Expand All @@ -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 }
Expand Down
15 changes: 14 additions & 1 deletion packages/fs/fs.walk/src/providers/stream.spec.ts
Expand Up @@ -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 {
Expand Down Expand Up @@ -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());
});
});
});
6 changes: 5 additions & 1 deletion packages/fs/fs.walk/src/providers/stream.ts
Expand Up @@ -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) { }
Expand Down
8 changes: 8 additions & 0 deletions packages/fs/fs.walk/src/readers/async.spec.ts
Expand Up @@ -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');

Expand Down
4 changes: 4 additions & 0 deletions packages/fs/fs.walk/src/readers/async.ts
Expand Up @@ -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');
Expand Down