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.

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: add new hooks api #3636

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 15 additions & 13 deletions lib/api/api-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,10 @@ class RequestHandler extends AsyncResource {
this.callback = callback
this.res = null
this.abort = null
this.resume = null
this.body = body
this.trailers = {}
this.context = null
this.trailers = {}
this.onInfo = onInfo || null
this.highWaterMark = highWaterMark
this.reason = null
Expand Down Expand Up @@ -85,10 +86,12 @@ class RequestHandler extends AsyncResource {
this.context = context
}

onHeaders (statusCode, rawHeaders, resume, statusMessage) {
const { callback, opaque, abort, context, responseHeaders, highWaterMark } = this
onResponseStart (resume) {
this.resume = resume
}

const headers = responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders)
onResponseHeaders (headers, statusCode) {
const { callback, opaque, abort, highWaterMark } = this

if (statusCode < 200) {
if (this.onInfo) {
Expand All @@ -97,11 +100,10 @@ class RequestHandler extends AsyncResource {
return
}

const parsedHeaders = responseHeaders === 'raw' ? util.parseHeaders(rawHeaders) : headers
const contentType = parsedHeaders['content-type']
const contentLength = parsedHeaders['content-length']
const contentType = headers['content-type']
const contentLength = headers['content-length']
const res = new Readable({
resume,
resume: this.resume,
abort,
contentType,
contentLength: this.method !== 'HEAD' && contentLength
Expand All @@ -124,21 +126,21 @@ class RequestHandler extends AsyncResource {
trailers: this.trailers,
opaque,
body: res,
context
context: this.context
})
}
}

onData (chunk) {
onResponseData (chunk) {
return this.res.push(chunk)
}

onComplete (trailers) {
util.parseHeaders(trailers, this.trailers)
onResponseEnd (trailers) {
Object.assign(this.trailers, trailers)
this.res.push(null)
}

onError (err) {
onResponseError (err) {
const { res, callback, body, opaque } = this

if (callback) {
Expand Down
5 changes: 4 additions & 1 deletion lib/core/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const {
} = require('./util')
const { channels } = require('./diagnostics.js')
const { headerNameLowerCasedRecord } = require('./constants')
const DecoratorHandler = require('../handler/decorator-handler')

// Verifies that a given path is valid does not contain control chars \x00 to \x20
const invalidPathRegex = /[^\u0021-\u00ff]/
Expand Down Expand Up @@ -187,7 +188,9 @@ class Request {

this.servername = servername || getServerName(this.host) || null

this[kHandler] = handler
this[kHandler] = handler instanceof DecoratorHandler
? handler
: new DecoratorHandler(handler)

if (channels.create.hasSubscribers) {
channels.create.publish({ request: this })
Expand Down
10 changes: 5 additions & 5 deletions lib/core/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -500,11 +500,11 @@ function assertRequestHandler (handler, method, upgrade) {
throw new InvalidArgumentError('handler must be an object')
}

if (typeof handler.onConnect !== 'function') {
if (typeof handler.onConnect !== 'function' && typeof handler.onRequestStart !== 'function') {
throw new InvalidArgumentError('invalid onConnect method')
}

if (typeof handler.onError !== 'function') {
if (typeof handler.onError !== 'function' && typeof handler.onResponseError !== 'function') {
throw new InvalidArgumentError('invalid onError method')
}

Expand All @@ -517,15 +517,15 @@ function assertRequestHandler (handler, method, upgrade) {
throw new InvalidArgumentError('invalid onUpgrade method')
}
} else {
if (typeof handler.onHeaders !== 'function') {
if (typeof handler.onHeaders !== 'function' && typeof handler.onResponseHeaders !== 'function') {
throw new InvalidArgumentError('invalid onHeaders method')
}

if (typeof handler.onData !== 'function') {
if (typeof handler.onData !== 'function' && typeof handler.onResponseData !== 'function') {
throw new InvalidArgumentError('invalid onData method')
}

if (typeof handler.onComplete !== 'function') {
if (typeof handler.onComplete !== 'function' && typeof handler.onResponseEnd !== 'function') {
throw new InvalidArgumentError('invalid onComplete method')
}
}
Expand Down
5 changes: 3 additions & 2 deletions lib/dispatcher/dispatcher-base.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,12 @@ class DispatcherBase extends Dispatcher {

return this[kDispatch](opts, handler)
} catch (err) {
if (typeof handler.onError !== 'function') {
if (typeof handler.onError !== 'function' && typeof handler.onResponseError !== 'function') {
throw new InvalidArgumentError('invalid onError method')
}

handler.onError(err)
handler.onError?.(err)
handler.onResponseError?.(err)

return false
}
Expand Down
2 changes: 1 addition & 1 deletion lib/global.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

// We include a version number for the Dispatcher API. In case of breaking changes,
// this version number must be increased to avoid conflicts.
const globalDispatcher = Symbol.for('undici.globalDispatcher.1')
const globalDispatcher = Symbol.for('undici.globalDispatcher.2')
const { InvalidArgumentError } = require('./core/errors')
const Agent = require('./dispatcher/agent')

Expand Down
119 changes: 117 additions & 2 deletions lib/handler/decorator-handler.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,23 @@
'use strict'

const assert = require('node:assert')
const util = require('../core/util')

function toRawHeaders (headers) {
const rawHeaders = []
if (headers != null) {
for (const [key, value] of Object.entries(headers)) {
rawHeaders.push(Buffer.from(key), Buffer.from(value))
}
}
return rawHeaders
}

module.exports = class DecoratorHandler {
#handler
#onCompleteCalled = false
#onErrorCalled = false
#resume = null

constructor (handler) {
if (typeof handler !== 'object' || handler === null) {
Expand All @@ -14,12 +26,88 @@ module.exports = class DecoratorHandler {
this.#handler = handler
}

// New API

onRequestStart (abort) {
this.#handler.onRequestStart?.(abort)
this.#handler.onConnect?.(abort)
}

onResponseStart (resume) {
let ret = true

if (this.#handler.onResponseStart?.(resume) === false) {
ret = false
}

this.#resume = resume

return ret
}

onResponseHeaders (headers, statusCode) {
let ret = true

if (this.#handler.onResponseHeaders?.(headers, statusCode) === false) {
ret = false
}

if (this.#handler.onHeaders) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we prevent to have both the old and new API?

Copy link
Member Author

Choose a reason for hiding this comment

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

How would we do that?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can check at dispatcher-base when calling dispatch. We already validate the handler, we can take advantage of validating the method as well if we want to avoid receiving both functions/method at the same time.

const rawHeaders = toRawHeaders(headers)
if (this.#handler.onHeaders(statusCode, rawHeaders, this.#resume) === false) {
ret = false
}
}
Comment on lines +51 to +60
Copy link
Member

Choose a reason for hiding this comment

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

Here we will have consistency conditions, isn't it?
Because if one of them returns false, and other true, we will continue reading chunks, causing that possibly one of them gets corrupted.

Shall we better allow one or the other but nut the two of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I follow. If one of them returns false then the return value is false.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. But then what would happen if e.g. onResponseHeaders returns false but onHeaders returns true?

I think we are allowing them to pass both APIs at the same time? Wouldn't that maybe cause inconsistencies?

Copy link
Member Author

Choose a reason for hiding this comment

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

No that's fine, if one returns false then it's paused.


return ret
}

onResponseData (data) {
let ret = true

if (this.#handler.onResponseData?.(data) === false) {
ret = false
Copy link
Member

Choose a reason for hiding this comment

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

Same as my previous comment

}

if (this.#handler.onData?.(data) === false) {
ret = false
}

return ret
}

onResponseEnd (trailers) {
this.#handler.onResponseEnd?.(trailers)

if (this.#handler.onComplete) {
const rawHeaders = toRawHeaders(trailers)
this.#handler.onComplete(rawHeaders)
}
}

onResponseError (err) {
this.#handler.onResponseError?.(err)
this.#handler.onError?.(err)
}

// Old API

onRequestSent (...args) {
return this.#handler.onRequestSent?.(...args)
}

onConnect (...args) {
this.#onErrorCalled = false
this.#onCompleteCalled = false

this.#handler.onRequestStart?.(args[0])
return this.#handler.onConnect?.(...args)
}

onError (...args) {
this.#onErrorCalled = true

this.#handler.onResponseError?.(args[0])
return this.#handler.onError?.(...args)
}

Expand All @@ -32,7 +120,7 @@ module.exports = class DecoratorHandler {

onResponseStarted (...args) {
assert(!this.#onCompleteCalled)
assert(!this.#onErrorCalled)
// assert(!this.#onErrorCalled)
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a bug in the h2 client that wasn't caught before.

Copy link
Member Author

Choose a reason for hiding this comment

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

@metcoder95: client-h2. calls onResponseStarted after onError

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, interesting; I imagine this happens if something failed before dispatching the request, or how do you find it?

Copy link
Member Author

Choose a reason for hiding this comment

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


return this.#handler.onResponseStarted?.(...args)
}
Expand All @@ -41,13 +129,34 @@ module.exports = class DecoratorHandler {
assert(!this.#onCompleteCalled)
assert(!this.#onErrorCalled)

return this.#handler.onHeaders?.(...args)
let ret

if (this.#handler.onResponseStart?.(args[2]) === false) {
// TODO (fix): Strictly speaking we should not call onRepsonseHeaders
// after this...
ret = false
}

if (this.#handler.onResponseHeaders) {
const headers = util.parseHeaders(args[1])
if (this.#handler.onResponseHeaders(headers, args[0]) === false) {
ret = false
}
}

if (this.#handler.onHeaders) {
const ret2 = this.#handler.onHeaders?.(...args)
ret ??= ret2
}

return ret
}

onData (...args) {
assert(!this.#onCompleteCalled)
assert(!this.#onErrorCalled)

this.#handler.onResponseData?.(args[0])
return this.#handler.onData?.(...args)
}

Expand All @@ -56,6 +165,12 @@ module.exports = class DecoratorHandler {
assert(!this.#onErrorCalled)

this.#onCompleteCalled = true

if (this.#handler.onResponseEnd) {
const headers = util.parseHeaders(args[0])
this.#handler.onResponseEnd(headers)
}

return this.#handler.onComplete?.(...args)
}

Expand Down
5 changes: 4 additions & 1 deletion lib/handler/redirect-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const { kBodyUsed } = require('../core/symbols')
const assert = require('node:assert')
const { InvalidArgumentError } = require('../core/errors')
const EE = require('node:events')
const DecoratorHandler = require('./decorator-handler')

const redirectableStatusCodes = [300, 301, 302, 303, 307, 308]

Expand Down Expand Up @@ -45,7 +46,9 @@ class RedirectHandler {
this.abort = null
this.opts = { ...opts, maxRedirections: 0 } // opts must be a copy
this.maxRedirections = maxRedirections
this.handler = handler
this.handler = handler instanceof DecoratorHandler
? handler
: new DecoratorHandler(handler)
this.history = []
this.redirectionLimitReached = false

Expand Down
37 changes: 0 additions & 37 deletions test/client-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -881,43 +881,6 @@ test('request onInfo callback headers parsing', async (t) => {
t.ok(true, 'pass')
})

test('request raw responseHeaders', async (t) => {
t = tspl(t, { plan: 4 })
const infos = []

const server = net.createServer((socket) => {
const lines = [
'HTTP/1.1 103 Early Hints',
'Link: </style.css>; rel=preload; as=style',
'',
'HTTP/1.1 200 OK',
'Date: Sat, 09 Oct 2010 14:28:02 GMT',
'Connection: close',
'',
'the body'
]
socket.end(lines.join('\r\n'))
})
after(() => server.close())

await promisify(server.listen.bind(server))(0)

const client = new Client(`http://localhost:${server.address().port}`)
after(() => client.close())

const { body, headers } = await client.request({
path: '/',
method: 'GET',
responseHeaders: 'raw',
onInfo: (x) => { infos.push(x) }
})
await body.dump()
t.strictEqual(infos.length, 1)
t.deepStrictEqual(infos[0].headers, ['Link', '</style.css>; rel=preload; as=style'])
t.deepStrictEqual(headers, ['Date', 'Sat, 09 Oct 2010 14:28:02 GMT', 'Connection', 'close'])
t.ok(true, 'pass')
})

test('request formData', async (t) => {
t = tspl(t, { plan: 1 })

Expand Down
Loading
Loading