-
Notifications
You must be signed in to change notification settings - Fork 545
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
base: main
Are you sure you want to change the base?
feat: add new hooks api #3636
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) { | ||
|
@@ -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) { | ||
const rawHeaders = toRawHeaders(headers) | ||
if (this.#handler.onHeaders(statusCode, rawHeaders, this.#resume) === false) { | ||
ret = false | ||
} | ||
} | ||
Comment on lines
+51
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we will have consistency conditions, isn't it? Shall we better allow one or the other but nut the two of them? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. But then what would happen if e.g. I think we are allowing them to pass both APIs at the same time? Wouldn't that maybe cause inconsistencies? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
|
||
|
@@ -32,7 +120,7 @@ module.exports = class DecoratorHandler { | |
|
||
onResponseStarted (...args) { | ||
assert(!this.#onCompleteCalled) | ||
assert(!this.#onErrorCalled) | ||
// assert(!this.#onErrorCalled) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @metcoder95: client-h2. calls onResponseStarted after onError There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I applied the assertions to the core request handling: |
||
|
||
return this.#handler.onResponseStarted?.(...args) | ||
} | ||
|
@@ -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) | ||
} | ||
|
||
|
@@ -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) | ||
} | ||
|
||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 callingdispatch
. We already validate thehandler
, we can take advantage of validating the method as well if we want to avoid receiving both functions/method at the same time.