From f47ad2095cf10ff81873f3183cb57defe8a621a7 Mon Sep 17 00:00:00 2001 From: Szymon Marczak Date: Tue, 2 Apr 2019 13:46:55 +0200 Subject: [PATCH 1/7] Throw on incomplete response --- source/as-promise.ts | 11 ++++++++++- source/as-stream.ts | 8 +++++++- source/errors.ts | 12 +++++++++++- test/timeout.ts | 25 +++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/source/as-promise.ts b/source/as-promise.ts index a619a0734..56538e007 100644 --- a/source/as-promise.ts +++ b/source/as-promise.ts @@ -4,7 +4,7 @@ import getStream from 'get-stream'; import is from '@sindresorhus/is'; import PCancelable from 'p-cancelable'; import requestAsEventEmitter from './request-as-event-emitter'; -import {HTTPError, ParseError, ReadError} from './errors'; +import {HTTPError, ParseError, ReadError, IncompleteResponseError} from './errors'; import {mergeOptions} from './merge'; import {reNormalizeArguments} from './normalize-arguments'; import {CancelableRequest, Options, Response} from './utils/types'; @@ -40,6 +40,15 @@ export default function asPromise(options: Options) { return; } + if (!response.complete) { + if (response.req.aborted) { + // Canceled while downloading - will throw a CancelError or TimeoutError + return; + } + + reject(new IncompleteResponseError(response, options)); + } + const limitStatusCode = options.followRedirect ? 299 : 399; response.body = data; diff --git a/source/as-stream.ts b/source/as-stream.ts index e6daf1f31..2ca580763 100644 --- a/source/as-stream.ts +++ b/source/as-stream.ts @@ -1,7 +1,7 @@ import {PassThrough as PassThroughStream} from 'stream'; import duplexer3 from 'duplexer3'; import requestAsEventEmitter from './request-as-event-emitter'; -import {HTTPError, ReadError} from './errors'; +import {HTTPError, ReadError, IncompleteResponseError} from './errors'; import {MergedOptions, Response} from './utils/types'; export default function asStream(options: MergedOptions) { @@ -39,6 +39,12 @@ export default function asStream(options: MergedOptions) { isFinished = true; + response.once('end', () => { + if (!(response as any).complete) { + proxy.emit('error', new IncompleteResponseError(response, options)); + } + }); + response.pipe(output); for (const destination of piped) { diff --git a/source/errors.ts b/source/errors.ts index e5891200c..41b24a36e 100644 --- a/source/errors.ts +++ b/source/errors.ts @@ -1,5 +1,5 @@ import urlLib from 'url'; -import http, {IncomingHttpHeaders} from 'http'; +import http, {IncomingMessage, IncomingHttpHeaders} from 'http'; import is from '@sindresorhus/is'; import {Response, Timings, Options} from './utils/types'; import {TimeoutError as TimedOutError} from './utils/timed-out'; @@ -52,6 +52,16 @@ export class ReadError extends GotError { } } +export class IncompleteResponseError extends GotError { + response: IncomingMessage; + + constructor(response: IncomingMessage, options: Options) { + super('Incomplete response', {}, options); + this.name = 'IncompleteResponseError'; + this.response = response; + } +} + export class ParseError extends GotError { body: string | Buffer; diff --git a/test/timeout.ts b/test/timeout.ts index 97feb5ce9..c3a7c70a9 100644 --- a/test/timeout.ts +++ b/test/timeout.ts @@ -52,6 +52,11 @@ const downloadHandler = (_request, response) => { slowDataStream().pipe(response); }; +const incorrectContentLength = (_request, response) => { + response.setHeader('content-length', 10); + response.end('Hello'); +}; + test('timeout option', withServer, async (t, server, got) => { server.get('/', defaultHandler); @@ -449,3 +454,23 @@ test('no memory leak when using socket timeout and keepalive agent', withServer, t.is(socket.listenerCount('timeout'), 0); }); + +test('throws on incomplete response - promise', withServer, async (t, server, got) => { + server.get('/', downloadHandler); + + await t.throwsAsync(got({ + timeout: {request: 500} + }), got.TimeoutError); +}); + +test('throws on incomplete response - promise #2', withServer, async (t, server, got) => { + server.get('/', incorrectContentLength); + + await t.throwsAsync(got(''), got.IncompleteResponseError); +}); + +test('throws on incomplete response - stream', withServer, async (t, server, got) => { + server.get('/', incorrectContentLength); + + await t.throwsAsync(getStream(got.stream('')), got.IncompleteResponseError); +}); From a5020c7303b907e8ba713f1ad30e91e0d3cbc7df Mon Sep 17 00:00:00 2001 From: Szymon Marczak Date: Tue, 2 Apr 2019 14:37:31 +0200 Subject: [PATCH 2/7] fixes --- source/as-promise.ts | 12 ++++-------- source/as-stream.ts | 8 +------- source/errors.ts | 12 +----------- test/timeout.ts | 25 +++++++++++++++---------- 4 files changed, 21 insertions(+), 36 deletions(-) diff --git a/source/as-promise.ts b/source/as-promise.ts index 56538e007..2824bdaf6 100644 --- a/source/as-promise.ts +++ b/source/as-promise.ts @@ -4,7 +4,7 @@ import getStream from 'get-stream'; import is from '@sindresorhus/is'; import PCancelable from 'p-cancelable'; import requestAsEventEmitter from './request-as-event-emitter'; -import {HTTPError, ParseError, ReadError, IncompleteResponseError} from './errors'; +import {HTTPError, ParseError, ReadError} from './errors'; import {mergeOptions} from './merge'; import {reNormalizeArguments} from './normalize-arguments'; import {CancelableRequest, Options, Response} from './utils/types'; @@ -40,13 +40,9 @@ export default function asPromise(options: Options) { return; } - if (!response.complete) { - if (response.req.aborted) { - // Canceled while downloading - will throw a CancelError or TimeoutError - return; - } - - reject(new IncompleteResponseError(response, options)); + if (response.req.aborted) { + // Canceled while downloading - will throw a CancelError or TimeoutError + return; } const limitStatusCode = options.followRedirect ? 299 : 399; diff --git a/source/as-stream.ts b/source/as-stream.ts index 2ca580763..e6daf1f31 100644 --- a/source/as-stream.ts +++ b/source/as-stream.ts @@ -1,7 +1,7 @@ import {PassThrough as PassThroughStream} from 'stream'; import duplexer3 from 'duplexer3'; import requestAsEventEmitter from './request-as-event-emitter'; -import {HTTPError, ReadError, IncompleteResponseError} from './errors'; +import {HTTPError, ReadError} from './errors'; import {MergedOptions, Response} from './utils/types'; export default function asStream(options: MergedOptions) { @@ -39,12 +39,6 @@ export default function asStream(options: MergedOptions) { isFinished = true; - response.once('end', () => { - if (!(response as any).complete) { - proxy.emit('error', new IncompleteResponseError(response, options)); - } - }); - response.pipe(output); for (const destination of piped) { diff --git a/source/errors.ts b/source/errors.ts index 41b24a36e..e5891200c 100644 --- a/source/errors.ts +++ b/source/errors.ts @@ -1,5 +1,5 @@ import urlLib from 'url'; -import http, {IncomingMessage, IncomingHttpHeaders} from 'http'; +import http, {IncomingHttpHeaders} from 'http'; import is from '@sindresorhus/is'; import {Response, Timings, Options} from './utils/types'; import {TimeoutError as TimedOutError} from './utils/timed-out'; @@ -52,16 +52,6 @@ export class ReadError extends GotError { } } -export class IncompleteResponseError extends GotError { - response: IncomingMessage; - - constructor(response: IncomingMessage, options: Options) { - super('Incomplete response', {}, options); - this.name = 'IncompleteResponseError'; - this.response = response; - } -} - export class ParseError extends GotError { body: string | Buffer; diff --git a/test/timeout.ts b/test/timeout.ts index c3a7c70a9..55039262f 100644 --- a/test/timeout.ts +++ b/test/timeout.ts @@ -52,11 +52,6 @@ const downloadHandler = (_request, response) => { slowDataStream().pipe(response); }; -const incorrectContentLength = (_request, response) => { - response.setHeader('content-length', 10); - response.end('Hello'); -}; - test('timeout option', withServer, async (t, server, got) => { server.get('/', defaultHandler); @@ -464,13 +459,23 @@ test('throws on incomplete response - promise', withServer, async (t, server, go }); test('throws on incomplete response - promise #2', withServer, async (t, server, got) => { - server.get('/', incorrectContentLength); + server.get('/', downloadHandler); + + const promise = got('').on('response', () => { + setTimeout(() => promise.cancel(), 500); + }); - await t.throwsAsync(got(''), got.IncompleteResponseError); + await t.throwsAsync(promise, got.CancelError); }); -test('throws on incomplete response - stream', withServer, async (t, server, got) => { - server.get('/', incorrectContentLength); +test.failing('throws on incomplete response - stream', withServer, async (t, server, got) => { + server.get('/', downloadHandler); + + const errorString = 'Foobar'; + + const stream = got.stream('').on('response', () => { + setTimeout(() => stream.destroy(new Error(errorString)), 500); + }); - await t.throwsAsync(getStream(got.stream('')), got.IncompleteResponseError); + await t.throwsAsync(getStream(stream), errorString); }); From cff31ee1595ee1eb0866565a6e20cd1b3d866450 Mon Sep 17 00:00:00 2001 From: Szymon Marczak Date: Tue, 2 Apr 2019 19:25:39 +0200 Subject: [PATCH 3/7] Fixes --- source/as-promise.ts | 2 +- source/as-stream.ts | 10 +++++++++- source/request-as-event-emitter.ts | 21 ++++++++++++++++++++- test/timeout.ts | 8 +++++--- 4 files changed, 35 insertions(+), 6 deletions(-) diff --git a/source/as-promise.ts b/source/as-promise.ts index 2824bdaf6..970021a09 100644 --- a/source/as-promise.ts +++ b/source/as-promise.ts @@ -40,7 +40,7 @@ export default function asPromise(options: Options) { return; } - if (response.req.aborted) { + if (response.canceled()) { // Canceled while downloading - will throw a CancelError or TimeoutError return; } diff --git a/source/as-stream.ts b/source/as-stream.ts index e6daf1f31..c7d650b7c 100644 --- a/source/as-stream.ts +++ b/source/as-stream.ts @@ -23,7 +23,15 @@ export default function asStream(options: MergedOptions) { const emitter = requestAsEventEmitter(options, input); // Cancels the request - proxy._destroy = emitter.abort; + proxy._destroy = (error, callback) => { + // Stop transmitting data, + // so the stream won't have an `end` event. + output.destroy(); + + // Abort safely + emitter.abort(); + callback(error); + }; emitter.on('response', (response: Response) => { const {statusCode} = response; diff --git a/source/request-as-event-emitter.ts b/source/request-as-event-emitter.ts index dcc88d73e..8bedeca02 100644 --- a/source/request-as-event-emitter.ts +++ b/source/request-as-event-emitter.ts @@ -26,10 +26,14 @@ export interface RequestAsEventEmitter extends EventEmitter { abort: () => void; } +export interface Abortable { + abort: () => void; +} + export default (options, input?: TransformStream) => { const emitter = new EventEmitter() as RequestAsEventEmitter; const redirects = [] as string[]; - let currentRequest: http.ClientRequest; + let currentRequest: http.ClientRequest | Abortable; let requestUrl: string; let redirectString: string; let uploadBodySize: number | undefined; @@ -120,6 +124,21 @@ export default (options, input?: TransformStream) => { gotOptions: options }; + // Cached requests don't have the request object (so we can't do `request.aborted`) + // so we need to create our own `canceled` property. + if (response.fromCache) { + response.canceled = () => false; + + currentRequest = { + abort: () => { + response.canceled = () => true; + response.destroy(); + } + }; + } else { + response.canceled = () => response.req.aborted; + } + const rawCookies = response.headers['set-cookie']; if (options.cookieJar && rawCookies) { await Promise.all(rawCookies.map(rawCookie => setCookie(rawCookie, response.url))); diff --git a/test/timeout.ts b/test/timeout.ts index 55039262f..29872b23c 100644 --- a/test/timeout.ts +++ b/test/timeout.ts @@ -450,7 +450,7 @@ test('no memory leak when using socket timeout and keepalive agent', withServer, t.is(socket.listenerCount('timeout'), 0); }); -test('throws on incomplete response - promise', withServer, async (t, server, got) => { +test('throws on incomplete (canceled) response - promise', withServer, async (t, server, got) => { server.get('/', downloadHandler); await t.throwsAsync(got({ @@ -458,7 +458,7 @@ test('throws on incomplete response - promise', withServer, async (t, server, go }), got.TimeoutError); }); -test('throws on incomplete response - promise #2', withServer, async (t, server, got) => { +test('throws on incomplete (canceled) response - promise #2', withServer, async (t, server, got) => { server.get('/', downloadHandler); const promise = got('').on('response', () => { @@ -468,7 +468,7 @@ test('throws on incomplete response - promise #2', withServer, async (t, server, await t.throwsAsync(promise, got.CancelError); }); -test.failing('throws on incomplete response - stream', withServer, async (t, server, got) => { +test('throws on incomplete (canceled) response - stream', withServer, async (t, server, got) => { server.get('/', downloadHandler); const errorString = 'Foobar'; @@ -479,3 +479,5 @@ test.failing('throws on incomplete response - stream', withServer, async (t, ser await t.throwsAsync(getStream(stream), errorString); }); + +test.todo('throws on incomplete (canceled) response - cached request'); From acf7e5c35a665cac38f981c829c70db5bf98b764 Mon Sep 17 00:00:00 2001 From: Szymon Marczak Date: Wed, 3 Apr 2019 14:24:25 +0200 Subject: [PATCH 4/7] fixes --- source/as-promise.ts | 2 +- source/request-as-event-emitter.ts | 21 +---------- test/cancel.ts | 57 ++++++++++++++++++++++++++++++ test/helpers/slow-data-stream.ts | 18 ++++++++++ test/timeout.ts | 50 +------------------------- 5 files changed, 78 insertions(+), 70 deletions(-) create mode 100644 test/helpers/slow-data-stream.ts diff --git a/source/as-promise.ts b/source/as-promise.ts index 970021a09..305aa6d4f 100644 --- a/source/as-promise.ts +++ b/source/as-promise.ts @@ -40,7 +40,7 @@ export default function asPromise(options: Options) { return; } - if (response.canceled()) { + if (response.req && response.req.aborted) { // Canceled while downloading - will throw a CancelError or TimeoutError return; } diff --git a/source/request-as-event-emitter.ts b/source/request-as-event-emitter.ts index 8bedeca02..dcc88d73e 100644 --- a/source/request-as-event-emitter.ts +++ b/source/request-as-event-emitter.ts @@ -26,14 +26,10 @@ export interface RequestAsEventEmitter extends EventEmitter { abort: () => void; } -export interface Abortable { - abort: () => void; -} - export default (options, input?: TransformStream) => { const emitter = new EventEmitter() as RequestAsEventEmitter; const redirects = [] as string[]; - let currentRequest: http.ClientRequest | Abortable; + let currentRequest: http.ClientRequest; let requestUrl: string; let redirectString: string; let uploadBodySize: number | undefined; @@ -124,21 +120,6 @@ export default (options, input?: TransformStream) => { gotOptions: options }; - // Cached requests don't have the request object (so we can't do `request.aborted`) - // so we need to create our own `canceled` property. - if (response.fromCache) { - response.canceled = () => false; - - currentRequest = { - abort: () => { - response.canceled = () => true; - response.destroy(); - } - }; - } else { - response.canceled = () => response.req.aborted; - } - const rawCookies = response.headers['set-cookie']; if (options.cookieJar && rawCookies) { await Promise.all(rawCookies.map(rawCookie => setCookie(rawCookie, response.url))); diff --git a/test/cancel.ts b/test/cancel.ts index 2e9b1e329..3e0f923b3 100644 --- a/test/cancel.ts +++ b/test/cancel.ts @@ -2,9 +2,11 @@ import {EventEmitter} from 'events'; import {Readable as ReadableStream} from 'stream'; import test from 'ava'; import pEvent from 'p-event'; +import getStream from 'get-stream'; // @ts-ignore import got, {CancelError} from '../source'; import withServer from './helpers/with-server'; +import slowDataStream from './helpers/slow-data-stream'; const prepareServer = server => { const emitter = new EventEmitter(); @@ -34,6 +36,14 @@ const prepareServer = server => { return {emitter, promise}; }; +const downloadHandler = (_request, response) => { + response.writeHead(200, { + 'transfer-encoding': 'chunked' + }); + response.flushHeaders(); + slowDataStream().pipe(response); +}; + test('does not retry after cancelation', withServer, async (t, server, got) => { const {emitter, promise} = prepareServer(server); @@ -145,3 +155,50 @@ test('recover from cancellation using error instance', async t => { await t.notThrowsAsync(recover); }); + +test('throws on incomplete (canceled) response - promise', withServer, async (t, server, got) => { + server.get('/', downloadHandler); + + await t.throwsAsync(got({ + timeout: {request: 500} + }), got.TimeoutError); +}); + +test('throws on incomplete (canceled) response - promise #2', withServer, async (t, server, got) => { + server.get('/', downloadHandler); + + const promise = got('').on('response', () => { + setTimeout(() => promise.cancel(), 500); + }); + + await t.throwsAsync(promise, got.CancelError); +}); + +test('throws on incomplete (canceled) response - stream', withServer, async (t, server, got) => { + server.get('/', downloadHandler); + + const errorString = 'Foobar'; + + const stream = got.stream('').on('response', () => { + setTimeout(() => stream.destroy(new Error(errorString)), 500); + }); + + await t.throwsAsync(getStream(stream), errorString); +}); + +// Note: it will throw, but the response is loaded already. +test('throws when canceling cached request', withServer, async (t, server, got) => { + server.get('/', (_request, response) => { + response.setHeader('Cache-Control', 'public, max-age=60'); + response.end(Date.now().toString()); + }); + + const cache = new Map(); + await got({cache}); + + const promise = got({cache}).on('response', () => { + promise.cancel(); + }); + + await t.throwsAsync(promise, got.CancelError); +}); diff --git a/test/helpers/slow-data-stream.ts b/test/helpers/slow-data-stream.ts new file mode 100644 index 000000000..ea11e56cc --- /dev/null +++ b/test/helpers/slow-data-stream.ts @@ -0,0 +1,18 @@ +import {PassThrough} from 'stream'; + +export default () => { + const slowStream = new PassThrough(); + let count = 0; + + const interval = setInterval(() => { + if (count++ < 10) { + slowStream.push('data\n'.repeat(100)); + return; + } + + clearInterval(interval); + slowStream.push(null); + }, 100); + + return slowStream; +}; diff --git a/test/timeout.ts b/test/timeout.ts index 29872b23c..295c904cc 100644 --- a/test/timeout.ts +++ b/test/timeout.ts @@ -7,23 +7,7 @@ import pEvent from 'p-event'; import delay from 'delay'; import got from '../source'; import withServer from './helpers/with-server'; - -const slowDataStream = () => { - const slowStream = new stream.PassThrough(); - let count = 0; - - const interval = setInterval(() => { - if (count++ < 10) { - slowStream.push('data\n'.repeat(100)); - return; - } - - clearInterval(interval); - slowStream.push(null); - }, 100); - - return slowStream; -}; +import slowDataStream from './helpers/slow-data-stream'; const requestDelay = 800; @@ -449,35 +433,3 @@ test('no memory leak when using socket timeout and keepalive agent', withServer, t.is(socket.listenerCount('timeout'), 0); }); - -test('throws on incomplete (canceled) response - promise', withServer, async (t, server, got) => { - server.get('/', downloadHandler); - - await t.throwsAsync(got({ - timeout: {request: 500} - }), got.TimeoutError); -}); - -test('throws on incomplete (canceled) response - promise #2', withServer, async (t, server, got) => { - server.get('/', downloadHandler); - - const promise = got('').on('response', () => { - setTimeout(() => promise.cancel(), 500); - }); - - await t.throwsAsync(promise, got.CancelError); -}); - -test('throws on incomplete (canceled) response - stream', withServer, async (t, server, got) => { - server.get('/', downloadHandler); - - const errorString = 'Foobar'; - - const stream = got.stream('').on('response', () => { - setTimeout(() => stream.destroy(new Error(errorString)), 500); - }); - - await t.throwsAsync(getStream(stream), errorString); -}); - -test.todo('throws on incomplete (canceled) response - cached request'); From 98c9710641db873c73e7086afaea9a813465243b Mon Sep 17 00:00:00 2001 From: Szymon Marczak Date: Wed, 3 Apr 2019 15:03:26 +0200 Subject: [PATCH 5/7] remove unnecessary code --- source/as-stream.ts | 5 ----- test/timeout.ts | 1 - 2 files changed, 6 deletions(-) diff --git a/source/as-stream.ts b/source/as-stream.ts index c7d650b7c..7f7fe3108 100644 --- a/source/as-stream.ts +++ b/source/as-stream.ts @@ -24,11 +24,6 @@ export default function asStream(options: MergedOptions) { // Cancels the request proxy._destroy = (error, callback) => { - // Stop transmitting data, - // so the stream won't have an `end` event. - output.destroy(); - - // Abort safely emitter.abort(); callback(error); }; diff --git a/test/timeout.ts b/test/timeout.ts index 295c904cc..7c8560f79 100644 --- a/test/timeout.ts +++ b/test/timeout.ts @@ -1,6 +1,5 @@ import http from 'http'; import net from 'net'; -import stream from 'stream'; import getStream from 'get-stream'; import test from 'ava'; import pEvent from 'p-event'; From 50b93b7aab6ec224a766f853f05935baf6796d2c Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Wed, 3 Apr 2019 15:16:38 +0200 Subject: [PATCH 6/7] Update as-stream.ts --- source/as-stream.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/as-stream.ts b/source/as-stream.ts index 7f7fe3108..7b3a8d7cb 100644 --- a/source/as-stream.ts +++ b/source/as-stream.ts @@ -24,8 +24,8 @@ export default function asStream(options: MergedOptions) { // Cancels the request proxy._destroy = (error, callback) => { - emitter.abort(); callback(error); + emitter.abort(); }; emitter.on('response', (response: Response) => { From eec7a52f986b509dcd047aeb53804edf8ca4f4a3 Mon Sep 17 00:00:00 2001 From: tobenna Date: Wed, 3 Apr 2019 21:13:32 +0200 Subject: [PATCH 7/7] Update test/helpers/slow-data-stream.ts Co-Authored-By: szmarczak <36894700+szmarczak@users.noreply.github.com> --- test/helpers/slow-data-stream.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/helpers/slow-data-stream.ts b/test/helpers/slow-data-stream.ts index ea11e56cc..eb39235f3 100644 --- a/test/helpers/slow-data-stream.ts +++ b/test/helpers/slow-data-stream.ts @@ -1,6 +1,6 @@ import {PassThrough} from 'stream'; -export default () => { +export default (): PassThrough => { const slowStream = new PassThrough(); let count = 0;