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

feat: add new hooks api #3636

wants to merge 7 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 21, 2024

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

@ronag
Copy link
Member Author

ronag commented Sep 21, 2024

@ronag ronag force-pushed the newhooks2 branch 6 times, most recently from 1fbf4d4 to f13993b Compare September 21, 2024 14:52
@ronag ronag requested review from Uzlopak, metcoder95 and mcollina and removed request for Uzlopak, metcoder95 and mcollina September 21, 2024 16:10
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

My understanding is that this is fully backward compatible?
I think we should bump the version on the global dispatcher symbol:

const globalDispatcher = Symbol.for('undici.globalDispatcher.1')

And then add an adapter to the "1" API, so that fetch keeps working.

Also... tests are missing.

@@ -32,7 +127,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.

@ronag
Copy link
Member Author

ronag commented Sep 22, 2024

My understanding is that this is fully backward compatible?

Yes

I think we should bump the version on the global dispatcher symbol:

Any specific reason why you think we should bump it?

Also... tests are missing.

👓

@ronag ronag force-pushed the newhooks2 branch 5 times, most recently from 86251ec to 4736a1b Compare September 22, 2024 09:07
@ronag
Copy link
Member Author

ronag commented Sep 22, 2024

@Uzlopak do you think you could help with tests?

@ronag ronag marked this pull request as draft September 22, 2024 17:11
@ronag
Copy link
Member Author

ronag commented Sep 22, 2024

This needs more work but I think it's mostly semver minor.

Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

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

The new APIs lgtm, left some minor comments 👍

Seems some tests will need adjustment as well documentation. I will check the h2 bug later this week

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

Comment on lines +51 to +60
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
}
}
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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants