feat: new hooks API #3054

ronag · 2024-04-04T14:59:02Z

While this is not breaking per se. It's pretty much unusable with existing handlers...

ShogunPanda · 2024-04-04T16:26:04Z

types/dispatcher.d.ts

+    onRequestEnd?(): void;
+
+    /** Invoked after response is starting to be processed */
+    onResponseStart?(controller: Controller): void;


I think you're not passing a controller here.
Also, in the onRequestStart above, you don't pass it so I would remove the comment at all.

So we have 2 different controllers, one for the request and one for the response, they may run in parallel.

ShogunPanda

One nit, rest LGTM!

ronag · 2024-04-04T17:51:17Z

lib/core/request.js


    if (channels.headers.hasSubscribers) {
      channels.headers.publish({ request: this, response: { statusCode, headers, statusText } })
    }

    try {
-      return this[kHandler].onHeaders(statusCode, headers, resume, statusText)
+      this[kHandler].onResponseHeaders?.(parseHeaders(headers), statusCode, statusText)


Here we are no longer pass raw headers. We parse them immediately. As far as I can tell this is what most users do and it's unfortunate to force middleware to work on raw headers.

@ShogunPanda @metcoder95 @mcollina any thoughts here? Is this a mistake (i.e. performance blocker) or just being pragmatic?

The latter. Let's be pragmatic.

+1 on pragmatism

metcoder95

LGTM, nice work; I can see the potential simplification of coordinating the flow control through the hooks thanks to the controller.

Just have some open questions left.

lib/core/request.js

metcoder95 · 2024-04-05T08:08:16Z

types/dispatcher.d.ts

+    /** Invoked after headers data has been received */
+    onResponseHeaders?(headers: Record<string, string>, statusCode: number, statusText?: string): void;
+    /** Invoked after response payload data is received. */
+    onResponseData?(chunk: Buffer | string): void;


Curious, how are we planning for the hook to hint the controller about backpressure, as it seems it does not receives the controller?

I was thinking the user saves a reference to the controller from the start hook. By I guess we could always send in the controller?

That's fair, we can keep it simple and keep the controller for that

codecov-commenter · 2024-04-05T10:29:51Z

Codecov Report

Attention: Patch coverage is 90.38462% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 93.50%. Comparing base (cae5625) to head (d1c5e47).
Report is 5 commits behind head on main.

Files	Patch %	Lines
lib/core/request.js	89.58%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3054      +/-   ##
==========================================
- Coverage   93.51%   93.50%   -0.02%     
==========================================
  Files          89       89              
  Lines       24331    24408      +77     
==========================================
+ Hits        22754    22822      +68     
- Misses       1577     1586       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ronag · 2024-04-06T09:58:29Z

@mcollina @ShogunPanda @metcoder95

Should we have separate controllers for request and response or just one for both or just the response? In theory the request and response can propagate independently hence it might make sense to be able to pause/resume them separately, on the other hand it's not a usual case and maybe just adds unnecessary complexity?
Should it be possible to pause/resume the request? Or should we always leave it to the server to apply back-pressure? One use case would to be able to throttle the client upload.

ShogunPanda · 2024-04-07T10:49:05Z

@mcollina @ShogunPanda @metcoder95

Should we have separate controllers for request and response or just one for both or just the response? In theory the request and response can propagate independently hence it might make sense to be able to pause/resume them separately, on the other hand it's not a usual case and maybe just adds unnecessary complexity?

I would keep one controller which acts on both. pause, resume and abort can act on both the request and response and the client can only be in either "sending request" or "receiving response" state at a time so no room for ambiguity.

Should it be possible to pause/resume the request? Or should we always leave it to the server to apply back-pressure? One use case would to be able to throttle the client upload.

I definitely would allow to also control the request flow.

metcoder95 · 2024-04-07T10:57:15Z

@mcollina @ShogunPanda @metcoder95

Should we have separate controllers for request and response or just one for both or just the response? In theory the request and response can propagate independently hence it might make sense to be able to pause/resume them separately, on the other hand it's not a usual case and maybe just adds unnecessary complexity?

I would keep one controller which acts on both. pause, resume and abort can act on both the request and response and the client can only be in either "sending request" or "receiving response" state at a time so no room for ambiguity.

Agree with @ShogunPanda towards a single controller, though I believe that most of the time implementers will look to control the stream flow over the response rather than the request; by providing a controller that allows them to also alter the flow of the request we might enable scenarios where users puts themselves into a weird spot and undici not being fully able to do its work accordingly.

The last being said, I'd prefer to keep the controller only for the response and we can evaluate later if there's a need for one over the response.

Should it be possible to pause/resume the request? Or should we always leave it to the server to apply back-pressure? One use case would to be able to throttle the client upload.

I definitely would allow to also control the request flow.

Couldn't we achieve that already with Dispatch#compose?
They can intercept the dispatch and throttle the body as they need

ronag · 2024-04-07T11:07:16Z

the client can only be in either "sending request" or "receiving response" state at a time so no room for ambiguity.

Actually, you can be sending and receiving at the same time. Though I'm not sure how common that is.

Couldn't we achieve that already with Dispatch#compose? They can intercept the dispatch and throttle the body as they need

Yes, that's also possible. Though less "nice".

@metcoder95 @ShogunPanda So in theory we could totally skip the request hooks (at least for now). However, there is one problem. We need to be able to abort the request before the response has been received (onResponseStart). That's the only place where we actually need to interact with the request.

i.e.

onRequestStart(controller: { abort: Function })

But I guess we can just implement that and leave the rest for the future. Or can you think of a nicer way? Preferably without any onRequestXXX hook.

One alternative is to keep onConnect (I'd prefer not).

ShogunPanda · 2024-04-07T12:50:33Z

the client can only be in either "sending request" or "receiving response" state at a time so no room for ambiguity.

Actually, you can be sending and receiving at the same time. Though I'm not sure how common that is.

Gosh, you are right. Why do I always forget about pipelining? XD

Couldn't we achieve that already with Dispatch#compose? They can intercept the dispatch and throttle the body as they need

Yes, that's also possible. Though less "nice".

@metcoder95 @ShogunPanda So in theory we could totally skip the request hooks (at least for now). However, there is one problem. We need to be able to abort the request before the response has been received (onResponseStart). That's the only place where we actually need to interact with the request.

i.e.

onRequestStart(controller: { abort: Function })

But I guess we can just implement that and leave the rest for the future. Or can you think of a nicer way? Preferably without any onRequestXXX hook.

One alternative is to keep onConnect (I'd prefer not).

Let's skip it for now. I would just say let's code it in a way we can introduce it later in a semver-minor.

metcoder95 · 2024-04-10T08:07:30Z

@metcoder95 @ShogunPanda So in theory we could totally skip the >request hooks (at least for now). However, there is one problem. We >need to be able to abort the request before the response has been >received (onResponseStart). That's the only place where we actually >need to interact with the request.

i.e.

onRequestStart(controller: { abort: Function })

But I guess we can just implement that and leave the rest for the future.

Agree and in sync with @ShogunPanda, let's do that 👍

ronag requested review from mcollina, ShogunPanda and metcoder95 April 4, 2024 14:59

ronag mentioned this pull request Apr 4, 2024

feat: new hooks API #3051

Closed

7 tasks

ronag force-pushed the new-hooks-simple branch 21 times, most recently from 145ad9e to 0444839 Compare April 4, 2024 15:54

ShogunPanda reviewed Apr 4, 2024

View reviewed changes

ShogunPanda approved these changes Apr 4, 2024

View reviewed changes

ronag commented Apr 4, 2024

View reviewed changes

metcoder95 reviewed Apr 5, 2024

View reviewed changes

ronag force-pushed the new-hooks-simple branch from 0444839 to b7887be Compare April 5, 2024 10:11

ronag force-pushed the new-hooks-simple branch from b7887be to 45e94a0 Compare April 5, 2024 10:22

feat: new hooks API

30cdcd3

ronag force-pushed the new-hooks-simple branch from 45e94a0 to 30cdcd3 Compare April 5, 2024 10:23

WIP

d1c5e47

This comment was marked as off-topic.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new hooks API #3054

feat: new hooks API #3054

ronag commented Apr 4, 2024 •

edited

ShogunPanda Apr 4, 2024

ronag Apr 5, 2024

ShogunPanda left a comment

ronag Apr 4, 2024 •

edited

ronag Apr 10, 2024

ShogunPanda Apr 10, 2024

metcoder95 Apr 11, 2024

metcoder95 left a comment

metcoder95 Apr 5, 2024

ronag Apr 5, 2024

metcoder95 Apr 7, 2024

codecov-commenter commented Apr 5, 2024 •

edited

ronag commented Apr 6, 2024 •

edited

ShogunPanda commented Apr 7, 2024

metcoder95 commented Apr 7, 2024

ronag commented Apr 7, 2024 •

edited

ShogunPanda commented Apr 7, 2024

metcoder95 commented Apr 10, 2024

This comment was marked as off-topic.

feat: new hooks API #3054

Are you sure you want to change the base?

feat: new hooks API #3054

Conversation

ronag commented Apr 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShogunPanda left a comment

Choose a reason for hiding this comment

ronag Apr 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metcoder95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 5, 2024 • edited

Codecov Report

ronag commented Apr 6, 2024 • edited

ShogunPanda commented Apr 7, 2024

metcoder95 commented Apr 7, 2024

ronag commented Apr 7, 2024 • edited

ShogunPanda commented Apr 7, 2024

metcoder95 commented Apr 10, 2024

This comment was marked as off-topic.

ronag commented Apr 4, 2024 •

edited

ronag Apr 4, 2024 •

edited

codecov-commenter commented Apr 5, 2024 •

edited

ronag commented Apr 6, 2024 •

edited

ronag commented Apr 7, 2024 •

edited