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

Expose Undici's ProxyAgent and setGlobalDispatcher within Node #43187

Open
pimterry opened this issue May 23, 2022 · 38 comments
Open

Expose Undici's ProxyAgent and setGlobalDispatcher within Node #43187

pimterry opened this issue May 23, 2022 · 38 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@pimterry
Copy link
Member

Originally posted by @pimterry in #42814 (comment)

As a motivating example: I'd love to automatically support global Fetch in https://www.npmjs.com/package/global-agent (drop-in lib to make all Node HTTP use the system proxy) but even though Node bundles Undici AFAICT this isn't possible without adding a full separate Undici dep to that lib, which would more than double its size, just [EDIT: largely] to ship code that's already present in Node itself.

This would be immediately solved if ProxyAgent and setGlobalDispatcher were exposed explicitly somewhere in future.

I've just done a quick test on main in Undici: exposing ProxyAgent & setGlobalDispatcher explicitly increases the Undici bundle size from 334.2kb to 336.0kb (+1.8kb / +0.5%).

Could these APIs be included and exposed in future? Agents for HTTP are a very core API that it would be useful to have usable out of the box, the equivalent functionality is usable OOTB for the legacy http module APIs, and 2kb is not a significant jump in bundle size for this functionality.

@targos
Copy link
Member

targos commented May 23, 2022

setGlobalDispatcher is exposed:

global[Symbol.for('undici.globalDispatcher.1')] = yourDispatcher;

@pimterry
Copy link
Member Author

Replying to the last comment from @mcollina in the previous issue:

I would recommend opening up a separate issue about this topic and bringing it to the TSC. I don't think the whole of Undici has the stability guarantees needed to be part of the Node.js LTS cycle yet.

This makes sense! I'm not aware of Undici's process around stability guarantees like this, but I can understand how there are constraints there.

I do understand this isn't a top top priority since installing & importing Undici elsewhere is a usable workaround, so there's certainly no rush just would justify shipping an unstable API unnecessarily. I do think that the current situation isn't a good end state though, and there are good options we can aim for to fix this, so it would be great to find agreement to aim in that direction in the medium term.

I'm not sure how to take anything to the TSC, but very happy to be included on any discussion of this any time.

@pimterry
Copy link
Member Author

global[Symbol.for('undici.globalDispatcher.1')] = yourDispatcher;

That's does work as a workaround, but it's not especially nice as the official API for this, and I think ProxyAgent needs to be available too to make this usable for global Fetch.

@mcollina
Copy link
Member

I don't think we should expose undici further until we want to consider fetch as stable.

@frank-dspeed

This comment was marked as off-topic.

@mcollina

This comment was marked as off-topic.

@sosoba
Copy link
Contributor

sosoba commented Dec 5, 2022

@mcollina
I don't think we should expose undici further until we want to consider fetch as stable.

2022-11-14, Version 19.1.0

@mcollina
Copy link
Member

mcollina commented Dec 5, 2022

That does not mark it stable, on purpose. It's the first step on that journey. This is something we might want to consider right now.

@mcollina
Copy link
Member

In the TSC meeeting of today, we decided that we are going to support HTTP_PROXY, HTTPS_PROXY and NO_PROXY env variables directly in Undici: nodejs/undici#1650.

@mcollina mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jan 18, 2023
@GabenGar
Copy link

How is it going to handle different casing rules?

@Ethan-Arrowood
Copy link
Contributor

Ethan-Arrowood commented Jul 19, 2024

At a minimum, I'd love it if I could use the workaround without calling fetch() first. I understand this is just the lazy-load feature in action and that has its own set of benefits. I may explore it a bit more and see if there's a simpler way to trigger the lazy loading

AndreasZeissner added a commit to wundergraph/cosmo that referenced this issue Aug 26, 2024
Setting the proxy relies on undici for now undici does not respect the
HTTP_PROXY vars, it's planned to build it in from v23 on by default:

nodejs/node#43187 (comment)

> My current plan is to add support for the HTTP_PROXY env variable
behind a flag in v22 (and possibly v20), then unflag in v23.

Using global dispatcher is the current way to go for native fetch,
      therefore undici needs to be added.

For connectrpc the agent will be passed down the chain:
/**
 * Options passed to the request() call of the Node.js built-in
 * http or https module.
 *

 example proxy trace:

 22:05:23 HTTPS POST
 localhost/auth/realms/cosmo/protocol/openid-connect/token              200    application/json  2.0k 202ms
 22:05:35 HTTP  POST
 localhost/wg.cosmo.platform.v1.PlatformService/DeleteFederatedGraph    200   application/proto   44b  64ms
AndreasZeissner added a commit to wundergraph/cosmo that referenced this issue Aug 26, 2024
Setting the proxy relies on undici for now undici does not respect the
HTTP_PROXY vars, it's planned to build it in from v23 on by default:

nodejs/node#43187 (comment)

> My current plan is to add support for the HTTP_PROXY env variable
behind a flag in v22 (and possibly v20), then unflag in v23.

Using global dispatcher is the current way to go for native fetch,
      therefore undici needs to be added.

For connectrpc the agent will be passed down the chain:
/**
 * Options passed to the request() call of the Node.js built-in
 * http or https module.
 *

 example proxy trace:

 22:05:23 HTTPS POST
 localhost/auth/realms/cosmo/protocol/openid-connect/token              200    application/json  2.0k 202ms
 22:05:35 HTTP  POST
 localhost/wg.cosmo.platform.v1.PlatformService/DeleteFederatedGraph    200   application/proto   44b  64ms
@gp-jameskyburz
Copy link

so, there is no way to set a dispatcher on a per-call basis yet?

The easiest way is to pass { dispatcher: x } on each undici call, otherwise there is a factory method if using a global dispatcher.

Below accesses the internals of undici to get Agent, the factory method uses either Pool, or Client.

/**
 * Use the undici internals shipped with Node.js
 */
function getUndiciInternals() {
  /* We need to call fetch for the globalDispatcher to be available */
  fetch().catch(() => {})
  const globalDispatcherSymbol = Symbol.for('undici.globalDispatcher.1')
  const agent = globalThis[globalDispatcherSymbol]
  /* get access to the factory to avoid needing to reference Pool, and Client */
  const agentFactorySymbol = Object.getOwnPropertySymbols(agent).find((x) => x.toString() === 'Symbol(factory)')
  const agentFactory = agent[agentFactorySymbol]
  const Agent = agent.constructor

  return {
    globalDispatcherSymbol,
    Agent,
    agentFactory,
  }
}

class MyDispatcher extends undici.Agent {
  constructor(options) {
    super({
      ...options,
      factory(url, options_) {
        // can pass other options here too :)
        return undici.agentFactory(url, options_)
      },
    })
  }
}
globalThis[undici.globalDispatcherSymbol] = new MyDispatcher()

@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 30, 2024
@mcollina
Copy link
Member

mcollina commented Aug 30, 2024

@nodejs/tsc I think we need to make a decision here. Do we want to expose the bundled undici or not? Even if not, we should at least provide some API to get the undici Agent because people are doing all kind of hacks to get it.

@nikelborm
Copy link

Press 👍🏻 if you think Undici should be exposed and 👎🏻 if not

@ert78gb
Copy link

ert78gb commented Aug 30, 2024

we should at least provide some API to get the undici Agent because people are doing all kind of hacks to get it.

Developers try to find a solution to their problem. We can call it hack.

This is why I suggested to expose the whole undici ~1 year ago.

@hansott
Copy link

hansott commented Aug 30, 2024

Would be nice to expose using node:undici (so that global fetch is the same as import { fetch } from 'node:undici')

@JamesKyburz
Copy link

Would be nice to expose using node:undici (so that global fetch is the same as import { fetch } from 'node:undici')

I like the idea of exporting node:undici fetch would be optional as it's already a global, just like it's optional to import process from node:process. The main benefit for me is not having to ship undici for AWS Lambda if all we want is to have a custom dispatcher.

mook-as added a commit to mook-as/rd that referenced this issue Sep 9, 2024
When fetching things during postinstall, add a much shorter timeout to
ensure things keep progressing.

If nodejs/node/issues/43187 gets resolved, we may be able to switch to
importing `node:undici` instead of adding another copy with possibly
incompatible API.

Signed-off-by: Mark Yen <mark.yen@suse.com>
mook-as added a commit to mook-as/rd that referenced this issue Sep 9, 2024
When fetching things during postinstall, add a much shorter timeout to
ensure things keep progressing.

If nodejs/node/issues/43187 gets resolved, we may be able to switch to
importing `node:undici` instead of adding another copy with possibly
incompatible API.

Signed-off-by: Mark Yen <mark.yen@suse.com>
mook-as added a commit to mook-as/rd that referenced this issue Sep 11, 2024
When fetching things during postinstall, actually retry downloads if they
fail partway through.

If nodejs/node/issues/43187 gets resolved, we may be able to switch to
importing `node:undici` instead of adding another copy with possibly
incompatible API.

Signed-off-by: Mark Yen <mark.yen@suse.com>
mook-as added a commit to mook-as/rd that referenced this issue Sep 11, 2024
When fetching things during postinstall, actually retry downloads if they
fail partway through.

If nodejs/node/issues/43187 gets resolved, we may be able to switch to
importing `node:undici` instead of adding another copy with possibly
incompatible API.

Signed-off-by: Mark Yen <mark.yen@suse.com>
mook-as added a commit to mook-as/rd that referenced this issue Sep 11, 2024
When fetching things during postinstall, actually retry downloads if they
fail partway through.

If nodejs/node/issues/43187 gets resolved, we may be able to switch to
importing `node:undici` instead of adding another copy with possibly
incompatible API.

Signed-off-by: Mark Yen <mark.yen@suse.com>
mook-as added a commit to mook-as/rd that referenced this issue Sep 12, 2024
When fetching things during postinstall, actually retry downloads if they
fail partway through.

If nodejs/node/issues/43187 gets resolved, we may be able to switch to
importing `node:undici` instead of adding another copy with possibly
incompatible API.

Signed-off-by: Mark Yen <mark.yen@suse.com>
@RedYetiDev
Copy link
Member

RedYetiDev commented Sep 13, 2024

9/4 Meeting

  • Expose Undici's ProxyAgent and setGlobalDispatcher within Node #43187
    • Two parts
      • For Node.js 23 do we want automatic support for HTTP_PROXY?, most frequent reason to
        have access to undici from Node.js versus installing undici
      • Problems exposing undici, cannot provide same stability guarantees
      • Michael, can we expose HTTP_PROXY without exposing undici
      • Marco, should we create a new namespace for undici
      • Paolo, in favor as well, yes lets create a new namespace
        • if we expose undici, then it would be experimental and we can break whenever. That would
          be an approach.
      • Marco, if requires new namespace, but at some point it would need to be as stable as the
        other APIs
      • Matteo, exposing has a lot of issues, keeping outside of core enabled a lot of
        experimentation and speed. Barrier to entry is lower. But also less stable, some back and
        forth
      • Paolo, already have similar situation with llhttp, 3 release lines, if we choose to expose
        undici can start to follow same approach. We would then have to start maintaining 3 different
        versions of undici.
      • Michael we are always in angst over thing that break, even if experimental so not really as
        easy as we break at will, better to wait until rate of change drops
      • Richard, llhttp is not quite the same as undici because we don’t expose it directly to users,
        ie we can swap (and did swap) the implementation behind the scenes.
      • Matteo has 3 parts
        • the part we bundle in Node.js which is the Web standard part
        • the part which is the public Undici API, quite large, has lots of agents, complex to address
          lots of use cases. For example dispatchers, but they expose some very low level API.
        • Generic question is people want to expose global dispatcher and access to dispatcher,
          those have been relatively stable in API, minus the fact that we just changed the expose API.

9/11 Meeting

  • Expose Undici's ProxyAgent and setGlobalDispatcher within Node #43187
  • We discussed last time, there was openness to including more of Undici but not all, remove
    from agenda for now.

@RedYetiDev RedYetiDev removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 13, 2024
@mbrevda
Copy link

mbrevda commented Sep 14, 2024

there was openness to including more of Undici but not all

Any more background on this?

@Fonger
Copy link

Fonger commented Sep 14, 2024

What about exposing normal Agent from undici too? I just need custom DNS lookup function and I shouldn't use ProxyAgent.

@timtucker-dte
Copy link

We wound up going down the path of including undici directly for most of what we're doing (using request and overriding the global dispatcher to use a custom lookup function)

With Node 22 including the web socket implementation, we went ahead and preemptively switched to that on Node 20 as well.

Having to include it manually isn't that big of a deal as long as it plays nicely together (i.e.: not needing to set 2 "global" dispatchers to cover anything that feeds through the native fetch).

@JakobJingleheimer
Copy link
Member

9/11 Meeting

Expose Undici's ProxyAgent and setGlobalDispatcher within Node #43187
We discussed last time, there was openness to including more of Undici but not all, remove from agenda for now.

Sooo is it happening?

@mcollina
Copy link
Member

@JakobJingleheimer it needs a champion.

hansott added a commit to AikidoSec/firewall-node that referenced this issue Nov 18, 2024
Fetch in node.js is powered by undici, node requires undici when fetch
is called the first time:

let fetchImpl;
// https://fetch.spec.whatwg.org/#fetch-method
ObjectDefineProperty(globalThis, 'fetch', {
  __proto__: null,
  configurable: true,
  enumerable: true,
  writable: true,
  value: function fetch(input, init = undefined) { // eslint-disable-line func-name-matching
    if (!fetchImpl) { // Implement lazy loading of undici module for fetch function
      const undiciModule = require('internal/deps/undici/undici');
      fetchImpl = undiciModule.fetch;
    }
    return fetchImpl(input, init);
  },
});

Only then will setGlobalDispatcher be available (albeit via a symbol
hack)

We need to wait until setGlobalDispatcher is properly exposed: nodejs/node#43187

We be able to call setGlobalDispatcher immediately, we call `fetch` with
an arguments and catch the error. We also need to wrap the fetch(<NO
ARGS>) call itself, even though there's `.catch(...)`. This is because
dd-trace throws an invalid URL error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
Development

Successfully merging a pull request may close this issue.