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

Extensible DispatchHandler #1338

Merged
merged 17 commits into from
Sep 6, 2022
Merged

Extensible DispatchHandler #1338

merged 17 commits into from
Sep 6, 2022

Conversation

arontsang
Copy link
Contributor

@arontsang arontsang commented Apr 13, 2022

This adds a plugin system that allows decorators to be added to the DispatchHandler as part of a pipeline.

I expect to see in future plugins such as (but not limited to):

This should support #491

@ronag
Copy link
Member

ronag commented Apr 14, 2022

I don't really see what this brings other than maybe better ergonomics? The dispatcher interface is already quite extensible? Could this live outside as an npm package?

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.

I see the need for better ergonomics for this, however I would not add any new globals for this. Could this be a feature of Agent instead?

@arontsang
Copy link
Contributor Author

arontsang commented Apr 15, 2022 via email

@arontsang
Copy link
Contributor Author

arontsang commented Apr 15, 2022 via email

@mcollina
Copy link
Member

How would the DispatcherDecorator work?

@arontsang
Copy link
Contributor Author

@mcollina Here is another commit showing what I mean arontsang@d54794f?diff=unified

In effect we add to the constructor of Dispatcher { interceptors: Record<string, DispatchInterceptor[]> }

Where type DispatchInterceptor = Dispatcher[kDispatch] => Dispatcher[kDispatch]
Where type Dispatcher[kDispatch] = (options: Dispatcher.DispatchOptions, handler: Dispatcher.DispatchHandlers) => boolean

We then reduce the array of DispatchInterceptors over this[kDispatch] to produce an intercepted dispatch method.

@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2022

Codecov Report

Merging #1338 (3add5df) into main (84f56f7) will decrease coverage by 0.28%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##             main    #1338      +/-   ##
==========================================
- Coverage   94.95%   94.67%   -0.29%     
==========================================
  Files          51       53       +2     
  Lines        4816     4862      +46     
==========================================
+ Hits         4573     4603      +30     
- Misses        243      259      +16     
Impacted Files Coverage Δ
lib/core/symbols.js 100.00% <ø> (ø)
lib/handler/DecoratorHandler.js 11.11% <11.11%> (ø)
lib/dispatcher-base.js 91.48% <61.11%> (-7.23%) ⬇️
index.js 100.00% <100.00%> (ø)
lib/agent.js 100.00% <100.00%> (ø)
lib/balanced-pool.js 98.85% <100.00%> (+0.01%) ⬆️
lib/client.js 96.98% <100.00%> (-0.01%) ⬇️
lib/handler/RedirectHandler.js 91.13% <100.00%> (ø)
lib/interceptor/redirectInterceptor.js 100.00% <100.00%> (ø)
lib/pool.js 100.00% <100.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@arontsang
Copy link
Contributor Author

I've just noticed that ProxyAgent could be replaced with a DispatchInterceptor quite trivially.

@arontsang arontsang requested a review from mcollina April 17, 2022 20:05
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.

Good work! Could you run the benchmarks before/after? Could you document this new feature?

lib/dispatcher-base.js Outdated Show resolved Hide resolved
types/dispatcher.d.ts Show resolved Hide resolved
lib/interceptor/redirect.js Outdated Show resolved Hide resolved
types/pool.d.ts Outdated Show resolved Hide resolved
@arontsang arontsang requested a review from mcollina April 21, 2022 17:18
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.

Good work!

'use strict'

const clearHeadersInterceptor = dispatch => {
const DecoratorHandler = require('undici/lib/handler/decorator')
Copy link
Member

Choose a reason for hiding this comment

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

This is an internal library, let's not document it. If it needs to be exposed, please use the module entry point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcollina I would prefer this to be exposed. I am actually trying to add support for NTLM undici, which doesn't seem like a great fit for being in mainline.

The only reason I am working on this is the fact that node.js has terrible support for socket based Auth. The socket is abstracted away completely.

Additionally, I suspect opening this up will allow for lots of extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also noticed that http headers being of type ByteArray means that each extension is likely to be to decode utf8 once each...

Copy link
Member

Choose a reason for hiding this comment

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

Go ahead and expose it in this PR.

I've also noticed that http headers being of type ByteArray means that each extension is likely to be to decode utf8 once each...

What do you mean? Can you make an example?

Copy link
Member

Choose a reason for hiding this comment

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

@kibertoad this needs fixing, we should not be deep-requiring.

lib/dispatcher-base.js Outdated Show resolved Hide resolved
@mcollina
Copy link
Member

@ronag PTAL

@arontsang
Copy link
Contributor Author

@ronag PTAL

@ronag Any chance you can take a look?

@ronag
Copy link
Member

ronag commented Apr 29, 2022

I'll have. A look next week.

@arontsang
Copy link
Contributor Author

arontsang commented Apr 29, 2022 via email

@ronag
Copy link
Member

ronag commented Apr 29, 2022

I think this can be achieved without modifying existing internals and I think I would maybe prefer that, i.e. implement a compose/decorate/intercept (or other name) function that implements a compose dispatcher that applies this logic. So it would essentially be possible to do the following without any modification of existing code:

const client = undici.intercept(new Client('https://localhost:3000', [ 
  dispatch => function insertHeader(opts, handler){
    opts.headers.push('Authorization', 'Bearer [Some token]')
    return dispatch(opts, handler)
  }
])

@ronag
Copy link
Member

ronag commented Apr 29, 2022

Or maybe:

const client = undici.compose(
  () => new Client('https://localhost:3000', 
  dispatch => function insertHeader(opts, handler){
    opts.headers.push('Authorization', 'Bearer [Some token]')
    return dispatch(opts, handler)
  }
)

@ronag
Copy link
Member

ronag commented Apr 29, 2022

undici.request(url, {
   decorate: [insertHeaders({ foo: 'bar' }, redirect({ maxRetries: 3 )]
})

@ronag
Copy link
Member

ronag commented Apr 29, 2022

This might need a few iterations and since it doesn't require any internal modifications we should consider whether or not we should first have it as an external package. Not saying we should. Just to consider.

@arontsang
Copy link
Contributor Author

undici.request(url, {
   decorate: [insertHeaders({ foo: 'bar' }, redirect({ maxRetries: 3 )]
})

This would not work for my use case. I NEED to apply the decorator AT the client level of an Agent.

The decorator needs to be scoped to a single TCP connection to be able to implement NTLM authentication.

@arontsang
Copy link
Contributor Author

Or maybe:

const client = undici.compose(
  () => new Client('https://localhost:3000', 
  dispatch => function insertHeader(opts, handler){
    opts.headers.push('Authorization', 'Bearer [Some token]')
    return dispatch(opts, handler)
  }
)

This works, but it would require an inordinate amount of boiler plate to bring up a full Agent with decorators at the right levels, Agent, Pool, Client etc...

@kibertoad
Copy link
Contributor

@arontsang Can you provide any details on

Well, if you are already thinking about it. I would like to change the
interface for the DispatchHandler to return headers as string[] rather than
ByteArray[].

This would help reduce the number of Utf8 decodes.

? I can't find where in the code this is happening

@kibertoad
Copy link
Contributor

kibertoad commented Sep 3, 2022

@mcollina @ronag I've resolved conflicts and addressed all the outstanding comments, could you please rereview?

upd: type tests are failing, I'll fix them shortly, shouldn't affect any of the runtime code

@kibertoad
Copy link
Contributor

@mcollina @ronag All green and ready for rereview.

'use strict'

const clearHeadersInterceptor = dispatch => {
const DecoratorHandler = require('undici/lib/handler/decorator')
Copy link
Member

Choose a reason for hiding this comment

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

@kibertoad this needs fixing, we should not be deep-requiring.

docs/api/Pool.md Outdated
@@ -19,6 +19,7 @@ Extends: [`ClientOptions`](Client.md#parameter-clientoptions)

* **factory** `(origin: URL, opts: Object) => Dispatcher` - Default: `(origin, opts) => new Client(origin, opts)`
* **connections** `number | null` (optional) - Default: `null` - The number of `Client` instances to create. When set to `null`, the `Pool` instance will create an unlimited amount of `Client` instances.
* **interceptors.Pool** `Array<DispatchInterceptor>` - Default: `[]` - A list of interceptors that are applied to the dispatch method. Additional logic can be applied (such as, but not limited to: 302 status code handling, authentication, cookies, compression and caching).
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear this is { interceptors: { Pool: [] } }

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed in all docs

@kibertoad
Copy link
Contributor

@mcollina Comments addressed!

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.

I would avoid introducing any more global settings, could you avoid them? Thanxk

@@ -42,7 +62,7 @@ class ProxyAgent extends DispatcherBase {

const connect = buildConnector({ ...opts.proxyTls })
this[kConnectEndpoint] = buildConnector({ ...opts.requestTls })
this[kClient] = new Client({ origin: opts.origin, connect })
this[kClient] = new Agent({ origin: opts.origin, connect })
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

const Client = require('./agent')
const Agent = require('./agent')

Naming was confusing, they were exactly same thing. Am I missing anything?

Copy link
Member

Choose a reason for hiding this comment

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

Seems more like the import was incorrect?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I'll fix it then

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@kibertoad
Copy link
Contributor

I would avoid introducing any more global settings, could you avoid them? Thanxk

@mcollina Do you mean new options for Client/Agent/Pool? Where would you place them instead?

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.

lgtm, that text was a leftover from a previous half-botched review

@kibertoad
Copy link
Contributor

@mcollina @ronag What's left before this can be merged?
Also it would be great to release a new version sometime after the merge, would allow our team to start working on actual Undici interceptors :D

@mcollina mcollina merged commit 33934f8 into nodejs:main Sep 6, 2022
@wong2
Copy link
Contributor

wong2 commented Sep 22, 2022

Does this support async logic in interceptors? For example, read credentials from the disk in a request interceptor and then add them to request headers.

@wong2
Copy link
Contributor

wong2 commented Sep 22, 2022

Also I find it annoying when extending request headers in a request interceptor, the opts.headers type is vague, according to typescript declaration they could be IncomingHttpHeaders | string[] | null, which makes it hard when extending...

@arontsang
Copy link
Contributor Author

arontsang commented Oct 11, 2022 via email

@arontsang
Copy link
Contributor Author

arontsang commented Oct 11, 2022 via email

metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022
* Create DispatchInterceptors

* Add Unit Tests, fix DispatchHandler typescript
Add documentation

* Switch to simple null check and shortcircuit bind

* Add typescript test for Dispatcher events

* Move build intecepted dispatch to top level

* Restore lost method

* Fix TS error

* Address code review comments

* Fix linting

* Type improvements

* Fix TS tests

* Address code review comments

* Fix TS test

* Fix types

* Address comments

* Fix client construction

Co-authored-by: Igor Savin <iselwin@gmail.com>
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* Create DispatchInterceptors

* Add Unit Tests, fix DispatchHandler typescript
Add documentation

* Switch to simple null check and shortcircuit bind

* Add typescript test for Dispatcher events

* Move build intecepted dispatch to top level

* Restore lost method

* Fix TS error

* Address code review comments

* Fix linting

* Type improvements

* Fix TS tests

* Address code review comments

* Fix TS test

* Fix types

* Address comments

* Fix client construction

Co-authored-by: Igor Savin <iselwin@gmail.com>
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.

6 participants