From 10cefe4d6cb723f56361b1d444f59ad5903df25d Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Mon, 12 Aug 2024 23:15:38 +0200 Subject: [PATCH] feat!: drop throwOnError --- docs/docs/api/Dispatcher.md | 1 - lib/api/api-request.js | 26 +++--- lib/api/api-stream.js | 79 ++++++++---------- lib/core/request.js | 3 - test/client-stream.js | 97 ---------------------- test/client.js | 147 ---------------------------------- test/issue-3136.js | 21 ----- test/node-test/async_hooks.js | 2 +- 8 files changed, 42 insertions(+), 334 deletions(-) delete mode 100644 test/issue-3136.js diff --git a/docs/docs/api/Dispatcher.md b/docs/docs/api/Dispatcher.md index 67819ecd525..933f4a730f8 100644 --- a/docs/docs/api/Dispatcher.md +++ b/docs/docs/api/Dispatcher.md @@ -201,7 +201,6 @@ Returns: `Boolean` - `false` if dispatcher is busy and further dispatch calls wo * **upgrade** `string | null` (optional) - Default: `null` - Upgrade the request. Should be used to specify the kind of upgrade i.e. `'Websocket'`. * **bodyTimeout** `number | null` (optional) - The timeout after which a request will time out, in milliseconds. Monitors time between receiving body data. Use `0` to disable it entirely. Defaults to 300 seconds. * **headersTimeout** `number | null` (optional) - The amount of time, in milliseconds, the parser will wait to receive the complete HTTP headers while not sending the request. Defaults to 300 seconds. -* **throwOnError** `boolean` (optional) - Default: `false` - Whether Undici should throw an error upon receiving a 4xx or 5xx response from the server. * **expectContinue** `boolean` (optional) - Default: `false` - For H2, it appends the expect: 100-continue header, and halts the request body until a 100-continue is received from the remote server #### Parameter: `DispatchHandler` diff --git a/lib/api/api-request.js b/lib/api/api-request.js index b988c2e7496..c9100ba6089 100644 --- a/lib/api/api-request.js +++ b/lib/api/api-request.js @@ -5,7 +5,6 @@ const { AsyncResource } = require('node:async_hooks') const { Readable } = require('./readable') const { InvalidArgumentError, RequestAbortedError } = require('../core/errors') const util = require('../core/util') -const { getResolveErrorBodyCallback } = require('./util') class RequestHandler extends AsyncResource { constructor (opts, callback) { @@ -13,7 +12,7 @@ class RequestHandler extends AsyncResource { throw new InvalidArgumentError('invalid opts') } - const { signal, method, opaque, body, onInfo, responseHeaders, throwOnError, highWaterMark } = opts + const { signal, method, opaque, body, onInfo, responseHeaders, highWaterMark } = opts try { if (typeof callback !== 'function') { @@ -54,7 +53,6 @@ class RequestHandler extends AsyncResource { this.trailers = {} this.context = null this.onInfo = onInfo || null - this.throwOnError = throwOnError this.highWaterMark = highWaterMark this.reason = null this.removeAbortListener = null @@ -118,20 +116,14 @@ class RequestHandler extends AsyncResource { this.callback = null this.res = res if (callback !== null) { - if (this.throwOnError && statusCode >= 400) { - this.runInAsyncScope(getResolveErrorBodyCallback, null, - { callback, body: res, contentType, statusCode, statusMessage, headers } - ) - } else { - this.runInAsyncScope(callback, null, null, { - statusCode, - headers, - trailers: this.trailers, - opaque, - body: res, - context - }) - } + this.runInAsyncScope(callback, null, null, { + statusCode, + headers, + trailers: this.trailers, + opaque, + body: res, + context + }) } } diff --git a/lib/api/api-stream.js b/lib/api/api-stream.js index 50f61632365..56e7621912c 100644 --- a/lib/api/api-stream.js +++ b/lib/api/api-stream.js @@ -1,11 +1,10 @@ 'use strict' const assert = require('node:assert') -const { finished, PassThrough } = require('node:stream') +const { finished } = require('node:stream') const { AsyncResource } = require('node:async_hooks') const { InvalidArgumentError, InvalidReturnValueError } = require('../core/errors') const util = require('../core/util') -const { getResolveErrorBodyCallback } = require('./util') const { addSignal, removeSignal } = require('./abort-signal') class StreamHandler extends AsyncResource { @@ -14,7 +13,7 @@ class StreamHandler extends AsyncResource { throw new InvalidArgumentError('invalid opts') } - const { signal, method, opaque, body, onInfo, responseHeaders, throwOnError } = opts + const { signal, method, opaque, body, onInfo, responseHeaders } = opts try { if (typeof callback !== 'function') { @@ -55,7 +54,6 @@ class StreamHandler extends AsyncResource { this.trailers = null this.body = body this.onInfo = onInfo || null - this.throwOnError = throwOnError || false if (util.isStream(body)) { body.on('error', (err) => { @@ -79,7 +77,7 @@ class StreamHandler extends AsyncResource { } onHeaders (statusCode, rawHeaders, resume, statusMessage) { - const { factory, opaque, context, callback, responseHeaders } = this + const { factory, opaque, context, responseHeaders } = this const headers = responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders) @@ -92,55 +90,42 @@ class StreamHandler extends AsyncResource { this.factory = null - let res + if (factory === null) { + return + } - if (this.throwOnError && statusCode >= 400) { - const parsedHeaders = responseHeaders === 'raw' ? util.parseHeaders(rawHeaders) : headers - const contentType = parsedHeaders['content-type'] - res = new PassThrough() + const res = this.runInAsyncScope(factory, null, { + statusCode, + headers, + opaque, + context + }) - this.callback = null - this.runInAsyncScope(getResolveErrorBodyCallback, null, - { callback, body: res, contentType, statusCode, statusMessage, headers } - ) - } else { - if (factory === null) { - return - } + if ( + !res || + typeof res.write !== 'function' || + typeof res.end !== 'function' || + typeof res.on !== 'function' + ) { + throw new InvalidReturnValueError('expected Writable') + } - res = this.runInAsyncScope(factory, null, { - statusCode, - headers, - opaque, - context - }) + // TODO: Avoid finished. It registers an unnecessary amount of listeners. + finished(res, { readable: false }, (err) => { + const { callback, res, opaque, trailers, abort } = this - if ( - !res || - typeof res.write !== 'function' || - typeof res.end !== 'function' || - typeof res.on !== 'function' - ) { - throw new InvalidReturnValueError('expected Writable') + this.res = null + if (err || !res.readable) { + util.destroy(res, err) } - // TODO: Avoid finished. It registers an unnecessary amount of listeners. - finished(res, { readable: false }, (err) => { - const { callback, res, opaque, trailers, abort } = this - - this.res = null - if (err || !res.readable) { - util.destroy(res, err) - } - - this.callback = null - this.runInAsyncScope(callback, null, err || null, { opaque, trailers }) + this.callback = null + this.runInAsyncScope(callback, null, err || null, { opaque, trailers }) - if (err) { - abort() - } - }) - } + if (err) { + abort() + } + }) res.on('drain', resume) diff --git a/lib/core/request.js b/lib/core/request.js index e4f05ad12da..a97dadc0a09 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -40,7 +40,6 @@ class Request { headersTimeout, bodyTimeout, reset, - throwOnError, expectContinue, servername }, handler) { @@ -86,8 +85,6 @@ class Request { this.bodyTimeout = bodyTimeout - this.throwOnError = throwOnError === true - this.method = method this.abort = null diff --git a/test/client-stream.js b/test/client-stream.js index c629f937e24..ccdbedf1b09 100644 --- a/test/client-stream.js +++ b/test/client-stream.js @@ -832,100 +832,3 @@ test('stream legacy needDrain', async (t) => { }) await t.completed }) - -test('stream throwOnError', async (t) => { - t = tspl(t, { plan: 3 }) - - const errStatusCode = 500 - const errMessage = 'Internal Server Error' - - const server = createServer((req, res) => { - res.writeHead(errStatusCode, { 'Content-Type': 'text/plain' }) - res.end(errMessage) - }) - after(() => server.close()) - - server.listen(0, async () => { - const client = new Client(`http://localhost:${server.address().port}`) - after(() => client.close()) - - client.stream({ - path: '/', - method: 'GET', - throwOnError: true, - opaque: new PassThrough() - }, ({ opaque: pt }) => { - pt.on('data', () => { - t.fail() - }) - return pt - }, (e) => { - t.strictEqual(e.status, errStatusCode) - t.strictEqual(e.body, errMessage) - t.ok(true, 'end') - }) - }) - - await t.completed -}) - -test('stream throwOnError, body is bigger than CHUNK_LIMIT', async (t) => { - t = tspl(t, { plan: 3 }) - - const errStatusCode = 500 - - const server = createServer((req, res) => { - res.writeHead(errStatusCode, { 'Content-Type': 'text/plain' }) - res.end(Buffer.alloc(128 * 1024 + 1)) - }) - after(() => server.close()) - - server.listen(0, async () => { - const client = new Client(`http://localhost:${server.address().port}`) - after(() => client.close()) - - client.stream({ - path: '/', - method: 'GET', - throwOnError: true, - opaque: new PassThrough() - }, ({ opaque: pt }) => { - pt.on('data', () => { - t.fail() - }) - return pt - }, (e) => { - t.strictEqual(e.status, errStatusCode) - t.strictEqual(e.body, undefined) - t.ok(true, 'end') - }) - }) - - await t.completed -}) - -test('steam throwOnError=true, error on stream', async (t) => { - t = tspl(t, { plan: 1 }) - - const server = createServer((req, res) => { - res.end('asd') - }) - after(() => server.close()) - - server.listen(0, async () => { - const client = new Client(`http://localhost:${server.address().port}`) - after(() => client.close()) - - client.stream({ - path: '/', - method: 'GET', - throwOnError: true, - opaque: new PassThrough() - }, () => { - throw new Error('asd') - }, (e) => { - t.strictEqual(e.message, 'asd') - }) - }) - await t.completed -}) diff --git a/test/client.js b/test/client.js index d353794dcb7..9cbda899f8c 100644 --- a/test/client.js +++ b/test/client.js @@ -320,153 +320,6 @@ test('basic get with query params partially in path', async (t) => { await t.completed }) -test('basic get returns 400 when configured to throw on errors (callback)', async (t) => { - t = tspl(t, { plan: 7 }) - - const server = createServer((req, res) => { - res.statusCode = 400 - res.end('hello') - }) - after(() => server.close()) - - server.listen(0, () => { - const client = new Client(`http://localhost:${server.address().port}`, { - keepAliveTimeout: 300e3 - }) - after(() => client.close()) - - const signal = new EE() - client.request({ - signal, - path: '/', - method: 'GET', - throwOnError: true - }, (err) => { - t.strictEqual(err.message, 'Response status code 400: Bad Request') - t.strictEqual(err.status, 400) - t.strictEqual(err.statusCode, 400) - t.strictEqual(err.headers.connection, 'keep-alive') - t.strictEqual(err.headers['content-length'], '5') - t.strictEqual(err.body, undefined) - }) - t.strictEqual(signal.listenerCount('abort'), 1) - }) - - await t.completed -}) - -test('basic get returns 400 when configured to throw on errors and correctly handles malformed json (callback)', async (t) => { - t = tspl(t, { plan: 6 }) - - const server = createServer((req, res) => { - res.writeHead(400, 'Invalid params', { 'content-type': 'application/json' }) - res.end('Invalid params') - }) - after(() => server.close()) - - server.listen(0, () => { - const client = new Client(`http://localhost:${server.address().port}`, { - keepAliveTimeout: 300e3 - }) - after(() => client.close()) - - const signal = new EE() - client.request({ - signal, - path: '/', - method: 'GET', - throwOnError: true - }, (err) => { - t.strictEqual(err.message, 'Response status code 400: Invalid params') - t.strictEqual(err.status, 400) - t.strictEqual(err.statusCode, 400) - t.strictEqual(err.headers.connection, 'keep-alive') - t.strictEqual(err.body, undefined) - }) - t.strictEqual(signal.listenerCount('abort'), 1) - }) - - await t.completed -}) - -test('basic get returns 400 when configured to throw on errors (promise)', async (t) => { - t = tspl(t, { plan: 6 }) - - const server = createServer((req, res) => { - res.writeHead(400, 'Invalid params', { 'content-type': 'text/plain' }) - res.end('Invalid params') - }) - after(() => server.close()) - - server.listen(0, async () => { - const client = new Client(`http://localhost:${server.address().port}`, { - keepAliveTimeout: 300e3 - }) - after(() => client.close()) - - const signal = new EE() - try { - await client.request({ - signal, - path: '/', - method: 'GET', - throwOnError: true - }) - t.fail('Should throw an error') - } catch (err) { - t.strictEqual(err.message, 'Response status code 400: Invalid params') - t.strictEqual(err.status, 400) - t.strictEqual(err.statusCode, 400) - t.strictEqual(err.body, 'Invalid params') - t.strictEqual(err.headers.connection, 'keep-alive') - t.strictEqual(err.headers['content-type'], 'text/plain') - } - }) - - await t.completed -}) - -test('basic get returns error body when configured to throw on errors', async (t) => { - t = tspl(t, { plan: 6 }) - - const server = createServer((req, res) => { - const body = { msg: 'Error', details: { code: 94 } } - const bodyAsString = JSON.stringify(body) - res.writeHead(400, 'Invalid params', { - 'Content-Type': 'application/json' - }) - res.end(bodyAsString) - }) - after(() => server.close()) - - server.listen(0, async () => { - const client = new Client(`http://localhost:${server.address().port}`, { - keepAliveTimeout: 300e3 - }) - after(() => client.close()) - - const signal = new EE() - try { - await client.request({ - signal, - path: '/', - method: 'GET', - throwOnError: true - }) - t.fail('Should throw an error') - } catch (err) { - t.strictEqual(err.message, 'Response status code 400: Invalid params') - t.strictEqual(err.status, 400) - t.strictEqual(err.statusCode, 400) - t.strictEqual(err.headers.connection, 'keep-alive') - t.strictEqual(err.headers['content-type'], 'application/json') - t.deepStrictEqual(err.body, { msg: 'Error', details: { code: 94 } }) - } - }) - - await t.completed -}) - test('basic head', async (t) => { t = tspl(t, { plan: 14 }) diff --git a/test/issue-3136.js b/test/issue-3136.js deleted file mode 100644 index 163433cb119..00000000000 --- a/test/issue-3136.js +++ /dev/null @@ -1,21 +0,0 @@ -const { request } = require('..') -const { test, after } = require('node:test') -const net = require('node:net') -const { once } = require('node:events') -const assert = require('node:assert') - -test('https://github.com/mcollina/undici/issues/3136', async (t) => { - const server = net.createServer((socket) => { - socket.write('HTTP/1.1 404 Not Found\r\n') - socket.write('Transfer-Encoding: chunked\r\n\r\n') - socket.write('\r\n') - }) - after(() => server.close()) - server.listen(0) - await once(server, 'listening') - await assert.rejects( - request(`http://localhost:${server.address().port}`, { - throwOnError: true - }) - ) -}) diff --git a/test/node-test/async_hooks.js b/test/node-test/async_hooks.js index 4814addde8d..63d42dd1082 100644 --- a/test/node-test/async_hooks.js +++ b/test/node-test/async_hooks.js @@ -161,7 +161,7 @@ test('async hooks client is destroyed', async (t) => { const client = new Client(`http://localhost:${server.address().port}`) t.after(client.destroy.bind(client)) - client.request({ path: '/', method: 'GET', throwOnError: true }, (err, { body }) => { + client.request({ path: '/', method: 'GET' }, (err, { body }) => { p.ifError(err) body.resume() body.on('error', (err) => {