Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: drop throwOnError #3451

Merged
merged 2 commits into from
Aug 14, 2024
Merged

feat!: drop throwOnError #3451

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10cefe4
feat!: drop throwOnError
metcoder95 Aug 12, 2024
f723cc2
feat: throw when passing throwOnError
metcoder95 Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion docs/docs/api/Dispatcher.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
26 changes: 9 additions & 17 deletions lib/api/api-request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,14 @@ 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) {
if (!opts || typeof opts !== 'object') {
throw new InvalidArgumentError('invalid opts')
}

const { signal, method, opaque, body, onInfo, responseHeaders, throwOnError, highWaterMark } = opts
const { signal, method, opaque, body, onInfo, responseHeaders, highWaterMark } = opts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw error if throwError === true

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we? Its a major, it means overall that it won't work if not documented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't hurt? Less suprising to people that don't update their code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair: f723cc2

try {
if (typeof callback !== 'function') {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
})
}
}

Expand Down
79 changes: 32 additions & 47 deletions lib/api/api-stream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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') {
Expand Down Expand Up @@ -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) => {
Expand All @@ -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)

Expand All @@ -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)

Expand Down
3 changes: 0 additions & 3 deletions lib/core/request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ class Request {
headersTimeout,
bodyTimeout,
reset,
throwOnError,
expectContinue,
servername
}, handler) {
Expand Down Expand Up @@ -86,8 +85,6 @@ class Request {

this.bodyTimeout = bodyTimeout

this.throwOnError = throwOnError === true

this.method = method

this.abort = null
Expand Down
97 changes: 0 additions & 97 deletions test/client-stream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
Loading
Loading