feat(ext/fetch): allow embedders to use hickory_dns_resolver instead of default GaiResolver

[sahandevs]

I've noticed while calling the fetch() function, a new thread spawns, which I don't think is necessary. We can reproduce this easily, by running the following script and watching new threads spawn and die using htop.

function sleep(ms: number) {
  return new Promise(r => setTimeout(r, ms));
}
console.log("pid=", Deno.pid);
while(true) {
  await sleep(Math.random() * 100 + 10000);
  console.log(Date.now());
  try {
    await fetch(`https://google-${parseInt(Math.random() * 100)}.com`);
  } catch (e) {}
}

export { }

So I investigated this issue, it seems hyper_util::builder uses something called GaiResolver in order to resolve hosts. GaiResolver works by calling stdlib's to_socket_addrs in a tokio::spawn_blocking which causes tokio to spawn some worker threads.

This PR fixes this issue by using trust_dns_resolver and replacing the GaiResolver.

[CLAassistant]

All committers have signed the CLA.

[bartlomieju]

Can you please provide some information what prompted you to open this PR? It is expected that there will be several Tokio threads spawned - Deno puts a limit of "blocking" threads at 32 (or 64 on Windows) that can be used for async FS operations and others, so these won't grow unbounded.

I'm skeptical of switching out the resolver as it might lead to a subtle bugs that will be hard to debug, we might want to consider having an env var, or a flag that switches the resolver though.

[sahandevs]

@bartlomieju
I'm using deno runtime in order run scripts in isolation (like how https://github.com/supabase/edge-runtime uses deno runtime to create edge function, albeit edge-runtime has its own runtime and only uses deno_core I think). I'm running every script with separate instances of deno runtime and tokio rt pair (which is single threaded) and
Because the number of scripts are high, I prefer limiting each script to a single thread so I won't need to do any extra book keeping (I've also limited or removed APIs that causes threads to spawn). Also because each script has it's own tokio rt, the 32 blocking thread is per script (this is for my use-case of course, it shouldn't be a problem with the normal deno use)

If you are worried about the subtle bugs (which I do agree may happen, especially because it's DNS), how about I add something like resolver: Resolver::Gai /* Resolver::Trust*/ in runtime init and an env variable (like your suggestion) for deno cli?

[bartlomieju]

@bartlomieju I'm using deno runtime in order run scripts in isolation (like how https://github.com/supabase/edge-runtime uses deno runtime to create edge function, albeit edge-runtime has its own runtime and only uses deno_core I think). I'm running every script with separate instances of deno runtime and tokio rt pair (which is single threaded) and Because the number of scripts are high, I prefer limiting each script to a single thread so I won't need to do any extra book keeping (I've also limited or removed APIs that causes threads to spawn). Also because each script has it's own tokio rt, the 32 blocking thread is per script (this is for my use-case of course, it shouldn't be a problem with the normal deno use)

If you are worried about the subtle bugs (which I do agree may happen, especially because it's DNS), how about I add something like resolver: Resolver::Gai /* Resolver::Trust*/ in runtime init and an env variable (like your suggestion) for deno cli?

Thanks for explanation, feels like something that was already tried in #22293 and #17166. I'm fine in making the resolver configurable for embedders, but for now I'd like to keep using the default resolver in Deno itself. If you could restructure the code to make resolver configurable in deno_fetch extension, I'll be happy to merge the PR, but the default implementation that the CLI uses should stay the same.

[sahandevs]

@bartlomieju I've applied your suggestion and made trust resolver optional. Would you please take a look?

[bartlomieju]

@sahandevs I'm off for a few days, I'll review and land this PR next week!

[bartlomieju]

Can you please rename this to fetch_dns_resolver?

[bartlomieju]

And this one to dns_resolver

[bartlomieju]

This should be handled gracefully with an error and not a panic

[bartlomieju]

Cool, thanks 👍