Skip to content

Commit

Permalink
Don't call beforeError hooks with HTTPError if the `throwHttpErro…
Browse files Browse the repository at this point in the history
…rs` option is `false` (#2104)
  • Loading branch information
szmarczak committed Aug 30, 2022
1 parent 9f06060 commit 3927348
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 3 deletions.
5 changes: 5 additions & 0 deletions documentation/3-streams.md
Expand Up @@ -239,6 +239,11 @@ To enable retrying when using streams, a retry handler must be attached.

When this event is emitted, you should reset the stream you were writing to and prepare the body again.

**Note:**
> - [`HTTPError`s](./8-errors.md#httperror) cannot be retried if [`options.throwHttpErrors`](./2-options.md#throwhttperrors) is `false`.
> This is because stream data is saved to `error.response.body` and streams can be read only once.
> - For the Promise API, there is no such limitation.
#### `retryCount`

**Type: `number`**
Expand Down
16 changes: 13 additions & 3 deletions source/core/index.ts
Expand Up @@ -797,6 +797,10 @@ export default class Request extends Duplex implements RequestEvents<Request> {
return;
}

// `HTTPError`s always have `error.response.body` defined.
// Therefore we cannot retry if `options.throwHttpErrors` is false.
// On the last retry, if `options.throwHttpErrors` is false, we would need to return the body,
// but that wouldn't be possible since the body would be already read in `error.response.body`.
if (options.isStream && options.throwHttpErrors && !isResponseOk(typedResponse)) {
this._beforeError(new HTTPError(typedResponse));
return;
Expand Down Expand Up @@ -1148,9 +1152,15 @@ export default class Request extends Duplex implements RequestEvents<Request> {

private async _error(error: RequestError): Promise<void> {
try {
for (const hook of this.options.hooks.beforeError) {
// eslint-disable-next-line no-await-in-loop
error = await hook(error);
if (error instanceof HTTPError && !this.options.throwHttpErrors) {
// This branch can be reached only when using the Promise API
// Skip calling the hooks on purpose.
// See https://github.com/sindresorhus/got/issues/2103
} else {
for (const hook of this.options.hooks.beforeError) {
// eslint-disable-next-line no-await-in-loop
error = await hook(error);
}
}
} catch (error_: any) {
error = new RequestError(error_.message, error_, this);
Expand Down
23 changes: 23 additions & 0 deletions test/hooks.ts
Expand Up @@ -1368,3 +1368,26 @@ test('does not throw on empty body when running afterResponse hooks', withServer
},
}));
});

test('does not call beforeError hooks on falsy throwHttpErrors', withServer, async (t, server, got) => {
server.get('/', (_request, response) => {
response.statusCode = 404;
response.end();
});

let called = false;

await got('', {
throwHttpErrors: false,
hooks: {
beforeError: [
error => {
called = true;
return error;
},
],
},
});

t.false(called);
});

0 comments on commit 3927348

Please sign in to comment.