Add a Timeout parameter

[philippebourcier]

I would like to be able to specify a timeout in milliseconds for the DoH request.

There are some fetchWithTimeout(URL,Timeout) functions available online that would probably work just fine...

[kimbo]

That sounds like a useful feature to me. I'll take a look at it 👍

[philippebourcier]

Wow @kimbo , cool, that was fast...

However, it seems your implementation doesn't work as expected for my use-case : the DoH server is down (ie: service dnsdist stop).
When I try to use it with a 75ms timeout setting on a server that is down (Failed to load resource: net::ERR_CONNECTION_REFUSED), it simply doesn't work... I still get a 2 seconds timeout (on Chrome).

You can see here : https://www.frnog.org/test/

[kimbo]

Wow @kimbo , cool, that was fast...

Well having a timeout is super useful. I would've already done it if I'd have thought of it before.

However, it seems your implementation doesn't work as expected for my use-case : the DoH server is down (ie: service dnsdist stop).
When I try to use it with a 75ms timeout setting on a server that is down (Failed to load resource: net::ERR_CONNECTION_REFUSED), it simply doesn't work... I still get a 2 seconds timeout (on Chrome).

Not sure if I understand you completely.
If the server isn't running, there won't be a timeout because the TCP connection will never actually happen. The query will never get sent, so it won't timeout waiting for a response.

[philippebourcier]

The concept of a timeout as I see it is to fail after TIMEOUT milliseconds, whatever is happening in the background... Here I shut the server in order to simulate a network outage of a DoH server that is not anycast or an outage of a whole AS, like CloudFlare encountered a few months ago...

In the UDP world of DNS, if the DNS client doesn't get an answer from the server after a short period (often less than 2 seconds), the client retries with another server. This is documented here (for Windows) : https://support.microsoft.com/en-au/help/2834226/net-dns-dns-client-resolution-timeouts and the same exists on the Linux-side of things : /etc/resolv.conf => options timeout:1 attempts:1 rotate.

This is why I talked about FetchWithTimeout in my first post...
https://davidwalsh.name/fetch-timeout
This implementation does exactly what I expected from a timeout setting (ie: cut the request so I can launch a new one on another server).

Here is how I use your library in my code (browser-based HTTP load-balancer), which implements a retry on another DoH server :
function LB(DoHost,DoHostFailover,host,timeout,_callback) {
const resolver = new doh.DohResolver(DoHost);
resolver.query(host,'CNAME','GET',{'Accept': 'text/plain'},500) // after 500ms, we try another DoH server
.then( response => { GetBestLB(response.answers,timeout,_callback); })
.catch( function(){ if(DoHostFailover!=null) LB(DoHostFailover,null,host,timeout,_callback); } );
}

[philippebourcier]

And btw :

If the server isn't running, there won't be a timeout because the TCP connection will never actually
happen. The query will never get sent, so it won't timeout waiting for a response.

If the DoH server is down or is having network issues, the browser will try to connect to it and timeout after 2 seconds (on Chrome)... and in the current implementation of the library, the fetch call is blocking (as opposed to FetchWithTimeout)... thus your current timeout setting is ignored.

[kimbo]

The concept of a timeout as I see it is to fail after TIMEOUT milliseconds, whatever is happening in the background

The way I see a timeout is that you fail after TIMEOUT, yes, unless some other fatal error occurs that causes you to fail before the timeout. A timeout isn't a "catch-all" for any error that occurs before the timeout.

[jacobgb24]

The way I see a timeout is that you fail after TIMEOUT, yes, unless some other fatal error occurs that causes you to fail before the timeout. A timeout isn't a "catch-all" for any error that occurs before the timeout.

I think the issue is that it isn't failing after TIMEOUT ms. Instead the code is blocking trying to connect to the server and that blocking causes TIMEOUT to be exceeded.

The query call should always be done after TIMEOUT ms have elapsed, regardless if there was a response or not.

[philippebourcier]

@kimbo But my problem here is that I've set a 500ms timeout on the DoH query... and since Chrome has a TCP timeout of 2s, the query runs for 2s and not 500 ms. The timeout is a maximum allowed, so this should not be occurring... There are no errors occuring before the 500 ms, it's just the browser trying to connect (and I read that on mobile, the wait can be infinite).
I don't know why, but it looks like in this situation the browser is not respecting your AbortController code... which is probably why the implementation of FetchWithTimeout uses a Promise.

Btw, even the guys writing the JS standard think there should be a timeout :
whatwg/fetch#20
whatwg/fetch#179
whatwg/fetch#951

@jacobgb24 I don't get your last sentence... there's no after/before to me... it's a parallel thing. On one thread you count ms, on another you try to fetch(). After TIMEOUT ms you kill the fetch() thread, if it did not finish before. That's all.

Here's a shorter version of the FetchWithTimeout :
besarthoxhaj/phoenix#33

[philippebourcier]

@kimbo :
In fact your implementation is doing exactly what I want but I found a bug.
It's a simple variable name issue.

If I try to :
console.log(opts.requestTimeout);
before :
if ('requestTimeout' in opts && opts.requestTimeout !== 0) {
in the block starting with :
if (self._mode === 'fetch') {

I get "undefined"... !

And if I add :
opts.requestTimeout=400;
... before my console.log(), I get exactly the result I was expecting.

So it seems there is an issue with your opts object...
When I dump it I see opts.timeout is 400, but there is no opts.requestTimeout.

So here is the fix :
*** doh.js 2020-03-19 18:11:10.883874462 +0000
--- npm/node_modules/dohjs/dist/doh.js 2020-03-19 17:49:54.081229247 +0000

---

*** 4051,4057 ****
headers: headers
};
if (timeout) {
! requestOptions.requestTimeout = timeout;
}
let data;
const request = transportModule.request(requestOptions, (response) => {
--- 4051,4057 ----
headers: headers
};
if (timeout) {
! requestOptions.timeout = timeout;
}
let data;
const request = transportModule.request(requestOptions, (response) => {

[philippebourcier]

There is still something a bit ugly though...

I've been forced to add "Access-Control-Allow-Headers : *" to my DoH server because your code adds the "requesttimeout" HTTP header to the DoH requests.
My browser was saying : "... has been blocked by CORS policy: Request header field requesttimeout is not allowed by Access-Control-Allow-Headers in preflight response."

Is there a reason why you are adding the requesttimeout variable and value as a HTTP header ?

[philippebourcier]

Adding "headersList.pop();" before "global.fetch(self._opts.url, {" works, but that's not what one could call state of the art... :)