Skip to content

Commit

Permalink
fix: fix sometimes encoded requests not ending properly
Browse files Browse the repository at this point in the history 
BREAKING CHANGES: `close` is no longer emitted from the miniget stream.
emitting `close` on a stream alters its internal state, and will prevent
it from properly finishing. this event was also a bit ambiguous in
miniget since it would only be emitted once at the end, and not for each
request. in comparsion, `request` and `response` can be emitted multiple
times.
  • Loading branch information
@fent
fent committed Nov 21, 2020
1 parent 0fe90c7 commit 5d4d149
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 31 deletions.
8 changes: 4 additions & 4 deletions src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const httpLibs: {
const redirectStatusCodes = new Set([301, 302, 303, 307, 308]);
const retryStatusCodes = new Set([429, 503]);

// `request`, `response`, `abort`, `close` left out, miniget will emit these.
// `request`, `response`, `abort`, left out, miniget will emit these.
const requestEvents = ['connect', 'continue', 'information', 'socket', 'timeout', 'upgrade'];
const responseEvents = ['aborted'];

Expand Down Expand Up @@ -188,9 +188,7 @@ function Miniget(url: string, options: Miniget.Options = {}): Miniget.Stream {

const onRequestClose = () => {
cleanup();
if (!retryRequest({})) {
stream.emit('close');
}
retryRequest({});
};

const cleanup = () => {
Expand All @@ -203,6 +201,7 @@ function Miniget(url: string, options: Miniget.Options = {}): Miniget.Stream {
};

const onData = (chunk: Buffer) => { downloaded += chunk.length; };

const onEnd = () => {
cleanup();
if (!reconnectIfEndedEarly()) {
Expand Down Expand Up @@ -283,6 +282,7 @@ function Miniget(url: string, options: Miniget.Options = {}): Miniget.Stream {
let destroyErr: Error;
const streamDestroy = (err?: Error) => {
activeRequest.destroy(err);
activeDecodedStream?.unpipe(stream);
activeDecodedStream?.destroy();
clearTimeout(retryTimeout);
};
Expand Down
52 changes: 25 additions & 27 deletions test/request-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -533,27 +533,26 @@ describe('Make a request', () => {
downloaded += chunk.length;
if (downloaded / filesize >= 0.3) {
destroyed = true;
destroy(req, res);
if (reconnects < 2) {
scope.get('/myfile')
.reply(206, () => fs.createReadStream(file, { start: downloaded }), {
'content-range': `bytes ${downloaded}-${filesize}/${filesize}`,
'content-length': `${filesize - downloaded}`,
'accept-ranges': 'bytes',
});
} else {
scope.done();
assert.equal(reconnects, 2);
assert.ok(destroyed);
assert.notEqual(downloaded, filesize);
done();
}
destroy(req, res);
}
});
stream.on('close', () => {
scope.done();
assert.equal(reconnects, 2);
assert.ok(destroyed);
assert.notEqual(downloaded, filesize);
done();
});
stream.on('end', () => {
// Does fire in node v12
// throw Error('should not end');
// done(Error('should not end'));
});
});
});
Expand Down Expand Up @@ -608,34 +607,35 @@ describe('Make a request', () => {

describe('that gets destroyed', () => {
describe('immediately', () => {
it('Does not end stream', () => {
it('Does not end stream', (done) => {
nock('http://anime.me')
.get('/')
.reply(200, 'ooooaaaaaaaeeeee');
const stream = miniget('http://anime.me');
stream.on('end', () => {
throw Error('`end` event should not be called');
done(Error('`end` event should not be called'));
});
stream.on('error', () => {
// Ignore error on node v10, 12.
});
stream.destroy();
done();
});
});
describe('after getting `request`', () => {
it('Does not start download, no `response` event', done => {
it('Does not start download, no `response` event', (done) => {
nock('https://friend.com')
.get('/yes')
.reply(200, '<html>my reply :)</html>');
const stream = miniget('https://friend.com/yes');
stream.on('end', () => {
throw Error('`end` event should not emit');
done(Error('`end` event should not emit'));
});
stream.on('response', () => {
throw Error('`response` event should not emit');
done(Error('`response` event should not emit'));
});
stream.on('data', () => {
throw Error('Should not read any data');
done(Error('Should not read any data'));
});
stream.on('error', () => {
// Ignore error on node v10, 12.
Expand All @@ -648,28 +648,28 @@ describe('Make a request', () => {
});
describe('after getting `response` but before end', () => {
it('Response does not give any data', (done) => {
const file = path.resolve(__dirname, 'video.flv');
const scope = nock('http://www.google1.com')
.get('/one')
.delayBody(100)
.reply(200, '<html></html>');
.replyWithFile(200, file);
const stream = miniget('http://www.google1.com/one');
stream.on('end', () => {
throw Error('`end` event should not emit');
console.trace();
done(Error('`end` event should not emit'));
});

stream.on('data', () => {
throw Error('Should not read any data');
done(Error('Should not read any data'));
});
const errorSpy = sinon.spy();
stream.on('error', errorSpy);
stream.on('close', () => {
stream.on('response', () => {
stream.destroy();
scope.done();
assert.ok(!errorSpy.called);
done();
});
stream.on('response', () => {
stream.destroy();
});
});
});

Expand All @@ -680,7 +680,7 @@ describe('Make a request', () => {
.reply(200, 'ooooaaaaaaaeeeee');
const stream = miniget('http://anime.me');
stream.on('end', () => {
throw Error('`end` event should not be called');
done(Error('`end` event should not be called'));
});
stream.on('error', () => {
// Ignore error on node v10, 12.
Expand Down Expand Up @@ -827,12 +827,10 @@ describe('Make a request', () => {
downloaded += chunk.length;
if (downloaded / filesize > 0.5) {
stream.destroy();
scope.done();
done();
}
});
stream.on('close', () => {
scope.done();
done();
});
});
});
});
Expand Down

0 comments on commit 5d4d149

Please sign in to comment.