Don't call beforeError hooks with HTTPError if the throwHttpErrors option is false (#2104)

Author: szmarczak

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`**

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);

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);
+});