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

dns round-robin interceptor + cache #3350

Closed
ronag opened this issue Jun 20, 2024 · 6 comments · Fixed by #3490
Closed

dns round-robin interceptor + cache #3350

ronag opened this issue Jun 20, 2024 · 6 comments · Fixed by #3490
Labels
enhancement New feature or request interceptors Pull requests or issues related to Dispatcher Interceptors

Comments

@ronag
Copy link
Member

ronag commented Jun 20, 2024

class Handler {
  // 1. resolve dns entries
  // 2. pick an ip for entry list
  // 3. const origin = new URL(opts.origin)
  // 4. origin.hostname = ip
  // 3. call dispatch({ ...opts, origin }, handler)
}

export const dnsInterceptor = dispatch => (opts, handler) => isDNSName(opts.origin.hostname)
  ? dispatch(opts, new Handler(opts, { dispatch, handler }))
  : dispatch(opts, handler)
@ronag ronag added the enhancement New feature or request label Jun 20, 2024
@ronag
Copy link
Member Author

ronag commented Jun 20, 2024

@metcoder95

@mcollina
Copy link
Member

this would need to implement Happy Eyeballs.

@metcoder95
Copy link
Member

This could be interesting, I was thinking about the same with the other closed issue but was unsure; if we are able to implement it with Happy Eyeballs will be really beneficial.

It could be good to also pick-up different strategies for distributing the load, possibly weighted or based on response.

@metcoder95 metcoder95 added the interceptors Pull requests or issues related to Dispatcher Interceptors label Aug 16, 2024
@metcoder95
Copy link
Member

I've put some hands on in this work, and I'm wondering what are your thoughts on several of these bits.

First, the interceptor might be made to be as lightweight as possible, so implementing fully the happy-eyeballs algorithm as per-spec might be complicated as there are several heuristics that are not easier to infer nor alter the behaviour of the dispatcher when certain conditions matches (e.g. timeouts on-connect, etc.).

Interceptors by design cannot interfere with the connect semantics, but rather with the overall request's one.

So for now, my initial idea is to:

  1. Resolve and dispatch based on origin
  • This can be decomposed into two steps, 1) on every dispatch we check for cached addresses, and only if not or ttl is expired we 2) lookup for addresses based on hostname
  1. Let the dispatcher take care of how to handle the underlying connections
    • e.g. most-likely the interceptor will only work with an Agent instance, as they handle multiple pool instance of single origin connections
  2. Basic happy-eyeballs upon ETIMEDOUT or ECONNREFUSED

My only concern is the async nature of the lookup and possible custom-submitted versions; we are somehow branching the chain removing the feedback for backpressure that dispatch provides.

Maybe just returning false its enough?

@ronag
Copy link
Member Author

ronag commented Aug 21, 2024

We did it like this:

https://github.com/nxtedition/nxt-undici/blob/main/lib/interceptor/dns.js
https://github.com/nxtedition/nxt-undici/blob/main/lib/interceptor/lookup.js

Where our default lookup function is:

function defaultLookup(origin, opts, callback) {
  callback(null, Array.isArray(origin) ? origin[Math.floor(Math.random() * origin.length)] : origin)
}

The DNS interceptor converts the origin to an array of possible origins and the lookup interceptor picks one if passed an array of origins.

@metcoder95
Copy link
Member

Ah, interesting; actually its not that far from what I'm currently doing 👍

Perfect, then it seems we are aligned, I'll also set the DNS settings in case no custom lookup is submitted and want to use default one.

So far, I'm starting with a Round Robin one, and simple version of HappyEyeballs. I'm making the lookup and pick customizable (so they can submit custom libs like cacheablelookup).

I'll open a PR maybe later Today 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request interceptors Pull requests or issues related to Dispatcher Interceptors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants