peter-murray / node-hue-api

Node.js Library for interacting with the Philips Hue Bridge and Lights
Apache License 2.0
1.18k stars 144 forks source link

Request: Honor "timeout" in initial connect() request (and API calls in general?) #212

Closed Codelica closed 2 years ago

Codelica commented 2 years ago

Currently the timeout property isn't honored for the initial local connection attempt made by connect(), and the 20000 ms default isn't applied either. As a result the timeout for an unreachable local bridge (ETIMEDOUT) is like 90 seconds.

Basically I'd just like to see the initial httpClient.request() in LocalBootstrap.js go from:

return httpClient.request({
            method: 'GET',
            url: `${baseUrl}api/config`,
            json: true,
            httpsAgent: INITIAL_HTTPS_AGENT
        }).then(res => {
...

to

return httpClient.request({
            method: 'GET',
            url: `${baseUrl}api/config`,
            json: true,
            httpsAgent: INITIAL_HTTPS_AGENT,
            timeout: getTimeout(timeout)
        }).then(res => {
...

I'll be connecting to a large number of local bridges at a business, so when one is unavailable 90 seconds can be an eternity :)

Thanks for considering...

EDIT: After further review, I'm not sure the timeout value passed into connect is actually being honored for local API calls either ? I'm setting a 5000 ms timeout in connect(), but all API calls to a bridge (that goes unreachable later) seem to timeout after 90 seconds also.

Trying to trace things down in HttpClientFetch.js, it looks like there is another timeout configured in the request config object itself on line 65 that is not related to the timeout value passed into connect() (which does seem to be found in the options object in the agent). This other timeout value (which always results in 0 from my limited tests?) seems to control the actual timeout of the node-fetch calls rather than what is in the agent options.

To confirm I just changed that timeout line from:

timeout: req.timeout || 0,

to

timeout: this.getAgent(url, req)?.options?.timeout || 0,

And calls started to honor my 5000ms timeout. Obviously not the best way to reference it but ¯(ツ)/¯ :)

peter-murray commented 2 years ago

Which version of the library are you using?

The timeout is currently being set inside the https agent on the LocalBootstrap; https://github.com/peter-murray/node-hue-api/blob/typescript/src/api/http/LocalBootstrap.ts#L87 for version 5.x

A similar approach is followed with the 4.x versions; https://github.com/peter-murray/node-hue-api/blob/4.x/lib/api/http/LocalBootstrap.js#L90

There are differences in the underlying Http clients, 5.x uses node-fetch and 4.x uses axios.

If you can provide some clarifications here as to where you are looking (files/branches) and the version you have issues with I can test and fix if necessary.

Codelica commented 2 years ago

Hello...

I'm using 5.0.0-beta.11 and setting my timeout in the via the 3rd connect() parameter, which I can see getting set in the https agent. It just seems that it's not used in the initial request() call and in subsequent node-fetch calls they seem to honor the other timeout sent in via options rather than the one set on the https agent. At least from what I see, setting things to 5000 ms on connect.

peter-murray commented 2 years ago

I have released version 5.0.0-beta.13 with modifications to not use the agent options and default to passing in the timeouts in the configuration for the fetch client.

Please let me know if there any further issues on this

Codelica commented 2 years ago

Thanks! Seems to be working well :)