From 39273481b67be41207c4efc31fc36e289eb53c88 Mon Sep 17 00:00:00 2001
From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com>
Date: Tue, 30 Aug 2022 07:20:00 +0200
Subject: [PATCH] Don't call `beforeError` hooks with `HTTPError` if the
 `throwHttpErrors` option is `false` (#2104)

---
 documentation/3-streams.md |  5 +++++
 source/core/index.ts       | 16 +++++++++++++---
 test/hooks.ts              | 23 +++++++++++++++++++++++
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/documentation/3-streams.md b/documentation/3-streams.md
index 9070e43da..5fe237cb9 100644
--- a/documentation/3-streams.md
+++ b/documentation/3-streams.md
@@ -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`**
diff --git a/source/core/index.ts b/source/core/index.ts
index edb12467d..4ead97ac5 100644
--- a/source/core/index.ts
+++ b/source/core/index.ts
@@ -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;
@@ -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);
diff --git a/test/hooks.ts b/test/hooks.ts
index fab442a11..22411376e 100644
--- a/test/hooks.ts
+++ b/test/hooks.ts
@@ -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);
+});