iobroker-community-adapters / ioBroker.boschindego

ioBroker Adapter for Bosch Indego
MIT License
2 stars 3 forks source link

Evaluate adding AXIOS timeout as default is 0 / no timeout #4

Closed mcm1957 closed 11 months ago

mcm1957 commented 11 months ago

It looks like currently axios calls do not specify an explicit timeout.

According to axios README:md (https://github.com/axios/axios/README.md) the default timeout is 0 === no timeout.

// timeout specifies the number of milliseconds before the request times out. // If the request takes longer than timeout, the request will be aborted. timeout: 1000, // default is 0 (no timeout)

Several adapters failed by locking up due to some network problem until a reasonable high timeout has been added to abort such request and log a communication error in those cases.

Please evealute / adapt code if applicable.

TA2k commented 11 months ago

No timeout for Axios is needed that's why there is not set by default. I prefer to let the Timeout on http level between client and server and handle the correct ETIMEDOUT in axios. A fix timeout can lead to information lost for slow connections.

Apollon77 commented 11 months ago

@TA2k What do you mean? Does axios use a default timeout in the meantime? If neót then you shpuld set a value (and if it is 60s), because else in case of network errors the request just hangs and so the whole adapter. It is common best practice in all communications to make sure to use proper timeoit values for the connection and also the response to prevent such issues.

TA2k commented 11 months ago

I think you mixing Axios timeout which is a response timeout and leads to ECONNABORTED and the connection timeout on network level/server timeout which leads to ETIMEDOUT Because I use intervals the adapter will not hang it will only opens up multiple connections until the connection timeouts on network level comes after X minutes depending on the server timeout I don't see the benefit especially for slow or overloaded APIs and the data from an adapter is nothing time critical and for the end user not relevant if it takes 10seconds or 2min. I assumed this also the reason why there is no default timeout on axios.

mcm1957 commented 11 months ago

There should be an overall timeout. Neither connection nor rrsponse must last foerver.

If you are using an interval you should ensure that one processing is completed,eventually by timeout,before the next interval starts It not desired tp have multiple processing loops in paralle as this put additional load on eventually overloaded apis an consumes local resources.

So axios timeouts should be shoerter or at least equal to interval. As an alternative the prossing loopcann be buit using timeouts.

@apollon77 please correct if this starement is incorrect or incomplete.

TA2k commented 11 months ago

There is every time a server timeout in place provided by the API Server. I personally prefer to follow the server timeout and wait until the server closes the connection. A response/client timeout will not reduce the load on the api because the request is already sent

The axios timeout should not be shorter in the case of short intervals this leads in high usage situation to the situation that all request are canceled on client side because they need more then 10seconds.

That's why if the request is not time critical it is though to define the correct client timeout and not follow the server timeout.

Apollon77 commented 11 months ago

Ok my personal experience would never trust server timeoits - especially also when considering the various possibilities for connection issue where a request "hngs" without ever having reached the server because it is running into a connect issue. So in my professional and private projects i always use client side timeouts because I had issues with these things too often.